feat(08-wasm): use 'clientID' as contract address #4939
Conversation
srdtrk
requested review from
colin-axner,
AdityaSripal and
damiannolan
as code owners
| clientStore = upws.subjectStore | ||
| } | ||
|
|
||
| store, ok := clientStore.(storeprefix.Store) |
There was a problem hiding this comment.
We are using this Store, this might help the reviewers
| // these fields are exported aliases for the payload fields passed to the wasm vm. | ||
| // these are used to specify callback functions to handle specific queries in the mock vm. | ||
| type ( | ||
| // CallbackFn types | ||
| QueryFn = queryFn | ||
| SudoFn = sudoFn | ||
| ) |
There was a problem hiding this comment.
I've removed the usage of these in my test in another PR but forgot to remove them from export_test.go. So I'm doing this here
There was a problem hiding this comment.
Thank you, @srdtrk. Nice tricks to get the client ID from the store!
| lastSlash := strings.LastIndex(prefixStr, "/") | ||
| if lastSlash == -1 { | ||
| return "", errorsmod.Wrapf(ErrRetrieveClientID, "prefix does not contain a slash") | ||
| } | ||
| secondLastSlash := strings.LastIndex(prefixStr[:lastSlash], "/") | ||
| if secondLastSlash == -1 { | ||
| return "", errorsmod.Wrapf(ErrRetrieveClientID, "prefix does not contain a second slash") | ||
| } | ||
|
|
||
| clientID := prefixStr[secondLastSlash+1 : lastSlash] | ||
|
|
||
| isClientID := strings.HasPrefix(clientID, exported.Wasm) | ||
| if !isClientID { | ||
| return "", errorsmod.Wrapf(ErrRetrieveClientID, "prefix does not contain a 08-wasm clientID") | ||
| } |
There was a problem hiding this comment.
I know we tend to prefer doing string parsing manually, but I think in this particular scenario a regex would probably make this code more readable. I was just playing a bit around with it and something like ^[ -~]*\/(08-wasm\-\d*){1}\/$ could do the trick. This could even be a separate function to test it more easily. [ -~] matches all ASCII characters, but we can tune this better to follow the spec.
There was a problem hiding this comment.
I'll move this validation steps for clientID to a validation helper in a follow up PR. Because we need the validation for MsgMigrateContract too. Can we discuss this again in that PR?
There was a problem hiding this comment.
Nice work @srdtrk! I am leaving my general approval. I think the parsing of the client ID code could be improved a bit readability wise + some more comments/docs, but otherwise looks good to me :)
| env := getEnv(ctx) | ||
| clientID, err := getClientID(clientStore) | ||
| if err != nil { | ||
| return nil, errorsmod.Wrapf(err, "failed to retrieve clientID for wasm contract instantiation") |
There was a problem hiding this comment.
should a test case be added for each of these errors?
There was a problem hiding this comment.
Added a test case in TestInitialize (Instantiate), TestVerifyClientMessage (Query), and TestUpdateState (Sudo)
| lastSlash := strings.LastIndex(prefixStr, "/") | ||
| if lastSlash == -1 { | ||
| return "", errorsmod.Wrapf(ErrRetrieveClientID, "prefix does not contain a slash") | ||
| } | ||
| secondLastSlash := strings.LastIndex(prefixStr[:lastSlash], "/") | ||
| if secondLastSlash == -1 { | ||
| return "", errorsmod.Wrapf(ErrRetrieveClientID, "prefix does not contain a second slash") | ||
| } | ||
|
|
||
| clientID := prefixStr[secondLastSlash+1 : lastSlash] | ||
|
|
||
| isClientID := strings.HasPrefix(clientID, exported.Wasm) | ||
| if !isClientID { | ||
| return "", errorsmod.Wrapf(ErrRetrieveClientID, "prefix does not contain a %s clientID", exported.Wasm) | ||
| } |
There was a problem hiding this comment.
I think this code could be simplified with strings.Split or something similar? I believe @chatton had some ideas.
Comments should indicate the expected format of the prefix
There was a problem hiding this comment.
Carlos also had the idea of using regexps. Should we create an issue for this?
…address' into serdar/issue#4928-use-contract-address
Description
closes: #4928
Commit Message / Changelog Entry
see the guidelines for commit messages. (view raw markdown for examples)
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/) or specification (x/<module>/spec/).godoccomments.Files changedin the Github PR explorer.Codecov Reportin the comment section below once CI passes.