chore: adding basic Msg service and SubmitTx rpc boilerplate #2086

charleenfei · 2022-08-23T12:01:40Z

Description

Please review #2133 first!

NOTE: i updated the tx name to be SubmitTx in order to avoid name collision with the current SendTx method in the ICA controller keeper. Open to other suggestions!

closes: #2052

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.

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer
Review Codecov Report in the comment section below once CI passes

charleenfei · 2022-08-24T10:09:08Z

docs/ibc/proto-docs.md

+| ----- | ---- | ----- | ----------- |
+| `owner` | [string](#string) |  |  |
+| `connection_id` | [string](#string) |  |  |
+| `timeout` | [string](#string) |  |  |


should timeout be Height or uint64 Timestamp here?

I think we should support both. Should be similar to MsgTransfer

I agree, I think we should support both going forward but the current implementation uses clienttypes.ZeroHeight.
I think we can only support the timeout height via the new execution path - i.e. existing auth modules using the SendTx go API directly won't be able to specify a height, right?

For SubmitTx we can use uint64 for timeout_timestamp and ibc.core.client.v1.Height for timeout_height like MsgTransfer as @colin-axner suggests! :)

colin-axner · 2022-08-24T10:56:18Z

proto/ibc/applications/interchain_accounts/controller/v1/tx.proto

+  string   owner                   = 1;
+  string   connection_id           = 2 [(gogoproto.moretags) = "yaml:\"connection_id\""];
+  string   timeout                 = 3;
+  repeated google.protobuf.Any msg = 4;


@damiannolan what's the reason for using Anys here? Shouldn't we just be taking in the ics27 packet data

I think if an end user wants to use x/group or x/gov they can't really construct the packet data type as it expects a proto encoded []byte of message data. If we use Any it can be defined in JSON and submitted for proposal execution. Essentially like it's done here. We would have a proto-JSON encoded MsgSubmitTx which has a nested array of ICA messages to be executed on the host.

Okay so this is an any which is decoded into packet data?

We cannot assume the counterparty uses any's for message delivery (this is SDK behaviour). I also suspect folks will want to make use of the memo field (it allows for a short term hack of enabling top level middleware for application handling, ie memo field: "swap:lp" would indicate, swap tokens then lp)

I think it'd be problematic to use Any to represent the messages delivered to the counterparty chain. These messages would need to be registered on the controller chain since the SDK will verify the Any URL is registered on the interface registry. Does that make sense?

Okay so this is an any which is decoded into packet data?

I was thinking that this would be an array Anys which would be encoded into packet data using SerializeCosmosTx.

These messages would need to be registered on the controller chain since the SDK will verify the Any URL is registered on the interface registry. Does that make sense?

This does make a lot of sense! It's a good point, do you know where in the SDK this would fail if for example you tried to provide an osmosis specific message type (swap) via a cosmos-hub(controller) group proposal?
I think this may be kind of problematic from a ux perspective...

I believe the issue occurs because UnpackAny will call UnpackInterfaces which will try to unpackany the nested Any, but this will fail to resolve. This is why the migration code has the mock solo machine client state/consensus state

colin-axner · 2022-08-24T10:58:17Z

NOTE: i updated the tx name to be SubmitTx in order to avoid name collision with the SendTx method in the transfer app. Open to other suggestions!

I like SubmitTx. As a side note, I don't think the name would conflict with ics20 since a user would submit a MsgTransfer and calling the ics20 function directly (it really should be private) would be transferkeeper.SendTx() compared to icacontrollerkeeper.SendTx()

charleenfei · 2022-08-24T11:37:13Z

ah sorry, i misread the error, there is already a SendTx in the controller keeper. If i understand this function correctly, this functionality is actually what should now be triggered by the new SubmitTx msg_server handler right? except we would take the Msg and parse the messages from that rather than the InterchainAccountPacketData

codecov-commenter · 2022-08-24T12:05:43Z

Codecov Report

Merging #2086 (864cbfb) into main (f8f879b) will decrease coverage by 0.12%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #2086      +/-   ##
==========================================
- Coverage   79.40%   79.28%   -0.13%     
==========================================
  Files         170      170              
  Lines       11837    11855      +18     
==========================================
  Hits         9399     9399              
- Misses       2022     2040      +18     
  Partials      416      416

Impacted Files	Coverage Δ
...nterchain-accounts/controller/keeper/msg_server.go	`83.33% <0.00%> (-16.67%)`	⬇️
...s/27-interchain-accounts/controller/types/codec.go	`0.00% <0.00%> (ø)`
...ps/27-interchain-accounts/controller/types/msgs.go	`58.82% <0.00%> (-32.09%)`	⬇️

colin-axner · 2022-08-24T14:06:04Z

ah sorry, i misread the error, there is already a SendTx in the controller keeper. If i understand this function correctly, this functionality is actually what should now be triggered by the new SubmitTx msg_server handler right? except we would take the Msg and parse the messages from that rather than the InterchainAccountPacketData

Ah yea, those names will collide as two functions for the same keeper. We want to leave the old function for backwards compatibility purposes. The new msg handler should perform the same functionality as the existing function but take in the arguments in form of MsgSubmitTx

AdityaSripal

It's ok to have owner be an address on the controller chain. It also needs to be a signer, so that user-controlled ICAs are secure

AdityaSripal · 2022-08-29T11:12:50Z

modules/apps/27-interchain-accounts/controller/types/msgs.go

+func NewMsgSubmitTx(connectionID, owner string, timeoutHeight clienttypes.Height, timeoutTimestamp uint64, msgs []*icatypes.InterchainAccountPacketData) *MsgSubmitTx {
+	return &MsgSubmitTx{
+		ConnectionId:     connectionID,
+		Owner:            owner,


This should be an AccAddress I believe because it is on the controller chain, we know it should be a valid address

is it better to cast it in the proto or check if its a valid addr in ValidateBasic?

AdityaSripal · 2022-08-29T11:13:11Z

modules/apps/27-interchain-accounts/controller/types/msgs.go

+
+// GetSigners implements sdk.Msg
+func (msg MsgSubmitTx) GetSigners() []sdk.AccAddress {
+	return []sdk.AccAddress{}


Suggested change

return []sdk.AccAddress{}

return []sdk.AccAddress{owner}

This needs to be authenticated by owner signature

yea i would fill out this function in the next PR along with validate basic :)

AdityaSripal · 2022-08-29T11:15:42Z

proto/ibc/applications/interchain_accounts/controller/v1/tx.proto

+  option (gogoproto.equal)           = false;
+  option (gogoproto.goproto_getters) = false;
+
+  string owner         = 1;


This can also be cast to sdk.Address

is it better to cast it in the proto or check if its a valid addr in ValidateBasic?

I think it is fine to leave as is, primarily because the owner is also the signer and all of our Msg types have the signer as an uncasted string (albeit I'm not sure why)

I think it makes sense just to apply the casting change to all our messages at once. The SDK does some different things nowadays anyways

I looked into this some more. Looks like the SDK intentionally chose to go with a string in 2020 (during amino -> proto migration). Might have been some concern with the gRPC gateway, nevertheless there's talk about removing reliance on the global bech32 config and moving it to be a param of the auth keeper, so you'd need access to the auth keeper to get the acc address (they want to change get signers to return a string instead of an address)

Lets keep with what we have. It'll be easier to deal with everything uniformly

AdityaSripal · 2022-08-29T13:32:34Z

Realize my requests are addressed in #2135 . Ignore the above!

…go into ics27/refactor-genesis-types

…pc_msgs

seantking

🚀 🚀

damiannolan · 2022-08-30T09:39:08Z

proto/ibc/applications/interchain_accounts/controller/v1/tx.proto

+  // Timeout timestamp in absolute nanoseconds since unix epoch.
+  // The timeout is disabled when set to 0.
+  uint64 timeout_timestamp = 4 [(gogoproto.moretags) = "yaml:\"timeout_timestamp\""];
+  ibc.applications.interchain_accounts.v1.InterchainAccountPacketData packet_data = 5;


We should add yaml tags and I think we can probs make this a value type instead of a pointer:
(gogoproto.nullable) = false

colin-axner

LGTM, left minor suggestions

modules/apps/27-interchain-accounts/controller/types/msgs.go

colin-axner · 2022-08-30T09:55:56Z

proto/ibc/applications/interchain_accounts/controller/v1/tx.proto

+
+  string owner         = 1;
+  string connection_id = 2 [(gogoproto.moretags) = "yaml:\"connection_id\""];
+  // Timeout height relative to the current block height.


I'm not sure we should be taking in a relative height here (especially since the timestamp is labeled as absolute). I think I'd prefer external UX to just figure out the correct timeout height

Is there a specific use case we can think of which needs a relative timeout?

i just grabbed the timeout parameters from the transfer app -- i guess the relative height might make sense if you want a time limit on the action relative to when you've submitted the tx (ie: you want to trigger a swap only within the next 24 hours)

modules/apps/27-interchain-accounts/controller/types/msgs.go

see followup discussion

* add protos/rpcs, boilerplate (cherry picked from commit f263794)

…2142) * add protos/rpcs, boilerplate (cherry picked from commit f263794) Co-authored-by: Charly <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

charleenfei added 3 commits August 23, 2022 13:58

add protos/rpcs, boilerplate

d20f2d2

typo

48e44b6

typo

3cacc48

charleenfei requested review from seantking, colin-axner, AdityaSripal, damiannolan, crodriguezvega and chatton as code owners August 23, 2022 12:01

crodriguezvega added the backport-to-v5.1.x label Aug 23, 2022

Merge branch 'main' into charly/2052_sendtx_rpc_msgs

ab10570

charleenfei changed the title ~~chore: adding basic Msg service and SubmitTx rpc boilerplate #2051~~ chore: adding basic Msg service and SubmitTx rpc boilerplate Aug 24, 2022

charleenfei commented Aug 24, 2022

View reviewed changes

colin-axner reviewed Aug 24, 2022

View reviewed changes

charleenfei added 4 commits August 24, 2022 15:44

update re comments

5eb8eb2

update msg

1748c3a

Merge branch 'main' into charly/2052_sendtx_rpc_msgs

c0a5259

merge main

aa6ce16

charleenfei force-pushed the charly/2052_sendtx_rpc_msgs branch from 5a65a82 to aa6ce16 Compare August 24, 2022 13:58

damiannolan mentioned this pull request Aug 25, 2022

Add MsgServer to interchain accounts controller #2026

Closed

11 tasks

damiannolan and others added 5 commits August 26, 2022 12:26

initial commit, adding all types

a4a7708

finish up correcting compile errors

81d74fd

goformat

bd25b55

Merge branch 'refactor_genesis_types' into charly/2052_sendtx_rpc_msgs

6e6fe62

update to new proto

fee8080

charleenfei force-pushed the charly/2052_sendtx_rpc_msgs branch from bf8e21d to fee8080 Compare August 26, 2022 13:37

AdityaSripal previously requested changes Aug 29, 2022

View reviewed changes

AdityaSripal self-requested a review August 29, 2022 13:32

charleenfei added 4 commits August 29, 2022 16:32

update proto

a605ba6

register interfaces update

a0a21ff

Merge branch 'main' into ics27/refactor-genesis-types

6c508cb

format

a9e44a1

charleenfei force-pushed the charly/2052_sendtx_rpc_msgs branch from 788b4b4 to a9e44a1 Compare August 29, 2022 14:59

charleenfei added 6 commits August 29, 2022 17:01

Merge branch 'main' into charly/2052_sendtx_rpc_msgs

a8bb9ca

update changelog

c897d0b

Merge branch 'ics27/refactor-genesis-types' of github.com:cosmos/ibc-…

ab6a796

…go into ics27/refactor-genesis-types

update changelog

656ca38

Merge branch 'ics27/refactor-genesis-types' into charly/2052_sendtx_r…

7f4274e

…pc_msgs

update msgSubmitTxResp

4eec1dc

seantking approved these changes Aug 29, 2022

View reviewed changes

charleenfei added 2 commits August 30, 2022 11:03

Merge branch 'main' into charly/2052_sendtx_rpc_msgs

e26f181

merge

864cbfb

damiannolan reviewed Aug 30, 2022

View reviewed changes

charleenfei added 2 commits August 30, 2022 11:46

update proto

1f0828a

update proto

679d47c

colin-axner approved these changes Aug 30, 2022

View reviewed changes

update ordering

26b6d6f

charleenfei enabled auto-merge (squash) August 30, 2022 10:10

charleenfei merged commit f263794 into main Aug 30, 2022

charleenfei deleted the charly/2052_sendtx_rpc_msgs branch August 30, 2022 10:40

mergify bot pushed a commit that referenced this pull request Aug 30, 2022

chore: adding basic Msg service and SubmitTx rpc boilerplate (#2086)

8018fe3

* add protos/rpcs, boilerplate (cherry picked from commit f263794)

mergify bot mentioned this pull request Aug 30, 2022

chore: adding basic Msg service and SubmitTx rpc boilerplate (backport #2086) #2142

Merged

chatton mentioned this pull request Nov 7, 2022

Add git cliff toml to support generation of changelog #2697

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: adding basic Msg service and SubmitTx rpc boilerplate #2086

chore: adding basic Msg service and SubmitTx rpc boilerplate #2086

charleenfei commented Aug 23, 2022 •

edited

Loading

charleenfei Aug 24, 2022 •

edited

Loading

colin-axner Aug 24, 2022

damiannolan Aug 24, 2022

colin-axner Aug 24, 2022

damiannolan Aug 24, 2022

colin-axner Aug 24, 2022

colin-axner Aug 24, 2022

damiannolan Aug 24, 2022

colin-axner Aug 24, 2022 •

edited

Loading

colin-axner commented Aug 24, 2022

charleenfei commented Aug 24, 2022 •

edited

Loading

codecov-commenter commented Aug 24, 2022 •

edited

Loading

colin-axner commented Aug 24, 2022

AdityaSripal left a comment

AdityaSripal Aug 29, 2022

charleenfei Aug 29, 2022

AdityaSripal Aug 29, 2022

charleenfei Aug 29, 2022

AdityaSripal Aug 29, 2022

charleenfei Aug 29, 2022

colin-axner Aug 29, 2022

colin-axner Aug 29, 2022

colin-axner Aug 30, 2022

AdityaSripal commented Aug 29, 2022

seantking left a comment

damiannolan Aug 30, 2022

colin-axner left a comment

colin-axner Aug 30, 2022

charleenfei Aug 30, 2022

chore: adding basic Msg service and SubmitTx rpc boilerplate #2086

chore: adding basic Msg service and SubmitTx rpc boilerplate #2086

Conversation

charleenfei commented Aug 23, 2022 • edited Loading

Description

charleenfei Aug 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colin-axner Aug 24, 2022 • edited Loading

Choose a reason for hiding this comment

colin-axner commented Aug 24, 2022

charleenfei commented Aug 24, 2022 • edited Loading

codecov-commenter commented Aug 24, 2022 • edited Loading

Codecov Report

colin-axner commented Aug 24, 2022

AdityaSripal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdityaSripal commented Aug 29, 2022

seantking left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colin-axner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charleenfei commented Aug 23, 2022 •

edited

Loading

charleenfei Aug 24, 2022 •

edited

Loading

colin-axner Aug 24, 2022 •

edited

Loading

charleenfei commented Aug 24, 2022 •

edited

Loading

codecov-commenter commented Aug 24, 2022 •

edited

Loading