[OTE-407][OTE-409][OTE-426]Update wasm bindings to use custom encoder #1809
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
affanv14
force-pushed
the
affan/update-cosmwasm
branch
from
c9706da to
1588285
Compare
affanv14
changed the title
protocol/wasmbinding/msg_plugin.go
Outdated
| return m.cancelOrder(ctx, contractAddr, contractMsg.CancelOrder) | ||
| } | ||
| return nil, nil, wasmvmtypes.InvalidRequest{Err: "Unknown custom message"} | ||
| func CustomEncoder(sender sdk.AccAddress, msg json.RawMessage) ([]sdk.Msg, error) { |
There was a problem hiding this comment.
Nit: rename to EncodeDydxCustomWasmMessage for parallelism with native encoders
There was a problem hiding this comment.
Also, can we document that sender here is enforced by wasmd to be the contractAddr, and point to relevant code here
There was a problem hiding this comment.
Did we address the 2nd comment above?
There was a problem hiding this comment.
| case customMessage.CancelOrder != nil: | ||
| return EncodeCancelOrder(sender, customMessage.CancelOrder) | ||
| default: | ||
| return nil, wasmvmtypes.InvalidRequest{Err: "Unknown Dydx Wasm Message"} | ||
| } |
There was a problem hiding this comment.
Can we add a TODO item in Linear to add E2E test for
- Successful contract dispatch of
PlaceOrdermesage - Contract cannot send
DepositToSubaccountmessage on behalf of another address?
Up to you on what's the best way to test this
| assert_eq!( | ||
| String::from_utf8_lossy(&json), | ||
| r#"{"place_order":{"subaccount_number":0,"client_id":0,"order_flags":0,"clob_pair_id":0,"side":1,"quantums":10000000000,"subticks":10000000000,"good_til_block_time":0,"time_in_force":1,"reduce_only":false,"client_metadata":0,"condition_type":1,"conditional_order_trigger_subticks":10000000000}}"# | ||
| ); |
There was a problem hiding this comment.
Are you sure you want to keep the enums (OrderSide::Buy, OrderTimeInForce::Ioc, OrderConditionType::StopLoss) as integers?
You can turn them into strings in JSON like this. But that's a matter of taste.
There was a problem hiding this comment.
That is what is expected from the protocol side - so wanted to keep things consistent
| r#"{"cancel_order":{"subaccount_number":0,"client_id":0,"order_flags":0,"clob_pair_id":0,"good_til_block_time":0}}"# | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Seems like you are not running cargo fmt. Would be good to do that in editor and do CI checks
There was a problem hiding this comment.
Good callout! added a task for myself to do this
|
LGTM |
affanv14
force-pushed
the
affan/update-cosmwasm
branch
from
77301c0 to
2c4634f
Compare
Changelist
Test Plan
Added rust unit tests
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.