-
Notifications
You must be signed in to change notification settings - Fork 457
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
Secret Network Permits Support #771
Open
luca992
wants to merge
24
commits into
chainapsis:master
Choose a base branch
from
luca992:secret-permits
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Always tries to create a permit instead of a viewing key: add user toggle (todo)
Only viewing keys will return errors in a valid result as data
Then update ui reactively according to permit support of the contract
and associated handlers. make sure the getSecret20ViewingKey can only return viewing keys
suggestViewingKey defaults to true. This is needed because all apps today expect viewing keys not permits. So if a user adds a token with permit manually, calling getSecret20ViewingKey will obviously return nothing. So the app needs to be able to request that the user overwrite the permit query authorization with a viewing key type authorization
luca992
added a commit
to luca992/keplr-wallet
that referenced
this pull request
Jun 27, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Permits support
If not permit support - keep current behavior.
Note:
Examples:
API suggestToken()
Add another option that is on by default for creating a permit instead of a VK:
Old:
Spec New:
New actual implementation:
Note:
suggestViewingKey: boolean = true
by default because if it werefalse
to instead suggest creating a permit it would basically be incompatible with all current apps. Since all current apps expect a viewing key to be made whensuggestToken
is called, not a permit.API getSecret20ViewingKeyOrPermit() (name TBD)
Spec:
Actual implementation:
getSecret20ViewingKeyOrPermit
I thinkgetSecret20QueryAuthorization
might be better. Because to me, using a viewing key or permit in a query is similar in concept to adding a authorization header to a rest call. That way instead of listing all the authorization types in the name it just broadly imply it can return any kind of authorization object. I am pretty sureauthorization
is the correct terminology of what a viewing key or a permit does.{ permit: Permit | undefined; viewing_key: string | undefined }
I could expose the internalQueryAuthorization
class and return that, which would debatably be cleaner. But up to you guys, just something to think about.