Open
Conversation
| pub gas: String, | ||
| } | ||
|
|
||
| // TODO: the order of these fields is VERY IMPORTANT, DO NOT CHANGE IT |
Contributor
There was a problem hiding this comment.
Suggested change
| // TODO: the order of these fields is VERY IMPORTANT, DO NOT CHANGE IT | |
| // The order of these fields is VERY IMPORTANT, DO NOT CHANGE IT |
Not a TODO?
0xekez
reviewed
Mar 12, 2023
Contributor
0xekez
left a comment
0xekez
left a comment
There was a problem hiding this comment.
i love the naming of the api and signature verification. just a couple questions from a first past.
| mut deps: DepsMut, | ||
| env: Env, | ||
| info: MessageInfo, | ||
| msg: InstantiateMsg, |
Contributor
There was a problem hiding this comment.
what do you think about making the InstantiateMsg an alias for Cw721BaseInstantiateMsg(ex)?
type InstantiateMsg = Cw721BaseInstantiateMsg;
that would remove the need for the first line of the function and imo helps convey the intent a little better.
| } | ||
|
|
||
| // the get_hash funtion will concat the address of the sender, the address of the 'to', the uri of the nft and the hash of the string | ||
| fn _get_hash(active: &str, passive: &str, uri: &str, chain_id: &str) -> Vec<u8> { |
Contributor
There was a problem hiding this comment.
could you expand a little on what these function arguments are? are active, passive, and uri intended to be additional metadata? i'm curious why you include chain id?
Comment on lines
+295
to
+296
| // TODO: modify this function to specify the others fields of the signing document | ||
| fn _get_sign_doc(signer: &str, message: &str) -> String { |
Contributor
There was a problem hiding this comment.
this and your types in state.rs are 🙏.
- is it possible to write an integration test for this? using cosmjs from the readme would be great.
- if you'd like, i think this signing logic would be a very helpful package to have. would you be interested in breaking it out into one?
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Cw4973 - Account-bound Tokens (ABT)
Cw4973 is implemented based on the description of EIP-4973. Some requirements in the description will be changed for suitable with CosmWasm characteristics.
In the contract, I also implement the signature based on ADR 036 and signature verification based on @cosmjs/amino.
Specification
Cw4973 is extended from Cw721-base with some modifications in execution section:
metadatastructures are unchanged.query functionsandquery messagesare unchanged.execute functionsandexecute messagesare removed.execute functionsandexecute messagesto the contract:GiveTakeUnequip