-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix (Read,Invoke) denied default provider handling #9067
Fix (Read,Invoke) denied default provider handling #9067
Conversation
When denying default providers was added, we had no special handling for Reads and Invokes. This lead to confusing error messages. The fix (#8853) involved checking on invokes. This check didn't apply to several types of calls (Read) as well as blocking invokes with providers applied. This PR fixes the logic to only deny providers when they are default providers. It also pushes the change into `getProviderFromSource`, which ensures that this behavior is handled the same way (and correctly) for both Invokes and Reads.
Confirm that this fixes #9060 as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM modulo nits. I'd like to see "token" removed from the arguments to getProviderFromSource
unless there's a need for it.
cases = append(cases, TT{ | ||
disableDefault: disableDefault, | ||
hasExplicit: hasExplicit, | ||
expectFail: disableDefault && !hasExplicit, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 surprisingly easy to read test matrix thanks to this conditional
This reverts commit 999da67.
When denying default providers was added, we had no special handling for
Reads and Invokes. This lead to confusing error messages. The fix (#8853)
involved checking on invokes. This check didn't apply to several types
of calls (Read) as well as blocking invokes with providers applied.
This PR fixes the logic to only deny providers when they are default
providers.
It also pushes the change into
getProviderFromSource
, which ensuresthat this behavior is handled the same way (and correctly) for both
Invokes and Reads.
Description
Fixes #9055
Fixes #9060
Checklist