-
Notifications
You must be signed in to change notification settings - Fork 228
Implement an option for contracts to set a last message marker #1399
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
Conversation
Added test that sends multiple contract msgs Small refactor on test utils
…last-message-tx # Conflicts: # cosmwasm/contracts/v1/compute-tests/test-compute-contract/src/contract.rs # cosmwasm/contracts/v1/compute-tests/test-compute-contract/src/msg.rs # x/compute/internal/keeper/msg_dispatcher.go # x/compute/internal/keeper/secret_contracts_query_test.go
|
Including scrtlabs/cosmos-sdk#293 and scrtlabs/cosmwasm#15 |
| // | ||
| d.keeper.GetLastMsgMarkerContainer().SetMarker(true) | ||
|
|
||
| // no handler is defined for marker - it's just to get here |
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.
Not sure I understand the comment
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.
If you look a few lines down, the dispatcher sends the message according to it's type to a handler. This is here to let future us know that no handler exists for this message, and if we let it pass through it will error out
| contractAddress sdk.AccAddress, txSender sdk.AccAddress, senderPrivKey crypto.PrivKey, execMsg []string, | ||
| isErrorEncrypted bool, isV1Contract bool, gas uint64, coin int64, shouldSkipAttributes ...bool, | ||
| ) ([]ExecResult, *ErrorResult) { | ||
| return execTxBuilderImpl(t, keeper, ctx, contractAddress, txSender, senderPrivKey, execMsg, isErrorEncrypted, isV1Contract, gas, sdk.NewCoins(sdk.NewInt64Coin("denom", coin)), -1, shouldSkipAttributes...) |
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.
The -1 here is super unclear for me it took a while to understand what is the here. My suggestion here is:
- The specific changes in the test utilities whould have better been in a different PR.
- Lets maybe create a contstant definition for the -1 here
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.
I really didn't want to touch this stuff so much, but it was pretty bad, so I just did the minimum to make things a bit more user friendly before a better rework
Co-authored-by: liorbond <[email protected]>
Co-authored-by: liorbond <[email protected]>
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
Implement an option for contracts to set a last message marker