WASM bindings refactoring #210
Merged
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
Contributor
iTiky
commented
Aug 22, 2022
iTiky
commented
- "rewards" embedded struct added for custom x/rewards query and msg handling (in case there would be other modules supporting WASM bindings, API won't change);
- CosmWasm custom queries dispatcher added for a future support of other modules with custom WASM bindings queries;
- E2E tests updated to test unknown custom msg and query;
Codecov Report
@@ Coverage Diff @@
## main #210 +/- ##
==========================================
+ Coverage 63.75% 64.30% +0.54%
==========================================
Files 52 59 +7
Lines 2337 2353 +16
==========================================
+ Hits 1490 1513 +23
+ Misses 779 775 -4
+ Partials 68 65 -3
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
fdymylja
reviewed
Aug 23, 2022
x/rewards/wasmbinding/msg_plugin.go
Outdated
|
|
||
| // Skip non module-related sub-message | ||
| if customMsg.Rewards == nil { | ||
| return p.wrappedMessenger.DispatchMsg(ctx, contractAddr, contractIBCPortID, msg) |
There was a problem hiding this comment.
I think this should return an error.
Because, if we are in customMsg.Rewards, and msg.Custom != nil, then it means:
- Either the user set msg.Custom && msg.SomethingElse -- which is incorrect by enum definition.
- Or it left msg.Custom empty (ex: {}), which is incorrect
I think here we should return an error.
fdymylja
reviewed
Aug 23, 2022
x/rewards/wasmbinding/msg_plugin.go
Outdated
| return p.withdrawContractRewards(ctx, contractAddr, *customMsg.Rewards.WithdrawRewards) | ||
| } | ||
|
|
||
| return nil, nil, sdkErrors.Wrap(rewardsTypes.ErrInvalidRequest, "unknown x/rewards custom message") |
There was a problem hiding this comment.
I'd put this in a default: , just personal opinion I think it makes code more readable.
fdymylja
approved these changes
Aug 23, 2022
* "rewards" embedded struct added for custom x/rewards query and msg handling (in case there would be other modules supporting WASM bindings, API won't change); * CosmWasm custom queries dispatcher added for a future support of other modules with custom WASM bindings queries; * E2E tests updated to test unknown custom msg and query;
iTiky
force-pushed
the
wasmb_refactoring
branch
from
1c760ca
to
8848e1d
Compare
…or all modules unlike multiple nested decorators and dispatchers)
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.