Add host cli to generate ica packet data #2297
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
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
This pull request introduces 1 alert when merging 1f12260 into 80ea89a - view on LGTM.com new alerts:
|
chatton
requested review from
seantking,
colin-axner,
AdityaSripal,
damiannolan,
crodriguezvega and
charleenfei
as code owners
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #2297 +/- ##
==========================================
- Coverage 79.54% 78.59% -0.95%
==========================================
Files 175 178 +3
Lines 12069 12323 +254
==========================================
+ Hits 9600 9685 +85
- Misses 2045 2206 +161
- Partials 424 432 +8
|
…rate-ica-packet-data
…rate-ica-packet-data
colin-axner
reviewed
Sep 20, 2022
colin-axner
reviewed
Sep 22, 2022
…rate-ica-packet-data
…cket-data' of https://github.com/cosmos/ibc-go into cian/issue#2243-add-a-ics27-host-cli-to-generate-ica-packet-data
|
@colin-axner thanks for the great suggestion! I adapted the link you sent and added support for both a single message in bytes, as well as a json array of messages in bytes. Tests were added for the new multi message scenario. |
charleenfei
approved these changes
Sep 23, 2022
damiannolan
approved these changes
Sep 26, 2022
| Short: "Generates ICA packet data.", | ||
| Long: `generate-packet-data accepts a message string and serializes it | ||
| into packet data which is outputted to stdout. It can be used in conjunction with send-tx" | ||
| which submits pre-built packet data containing messages to be executed on the host chain. |
There was a problem hiding this comment.
Would it be possible to add an example here? Maybe we can just use the regular bank transfer message used in the example in the ica-demo repo?
'{
"@type":"/cosmos.bank.v1beta1.MsgSend",
"from_address":"cosmos15ccshhmp0gsx29qpqq6g4zmltnnvgmyu9ueuadh9y2nc5zj0szls5gtddz",
"to_address":"cosmos10h9stc5v6ntgeygf5xf945njqq5h32r53uquvw",
"amount": [
{
"denom": "stake",
"amount": "1000"
}
]
}'
There was a problem hiding this comment.
great suggestions, I'll add this as an example
There was a problem hiding this comment.
@crodriguezvega I added this example and also a multi message example.
| assertionFn func(t *testing.T, msgs []sdk.Msg) | ||
| }{ | ||
| { | ||
| name: "multi message", |
There was a problem hiding this comment.
Suggested change
| name: "multi message", | |
| name: "packet data generation succeeds (MsgDelegate & MsgSend)", |
| banktypes.RegisterInterfaces(registry) | ||
| }, | ||
| assertionFn: func(t *testing.T, msgs []sdk.Msg) { | ||
| assertMsgDelegate(t, msgs, 0) |
There was a problem hiding this comment.
Maybe a nit, but instead of passing the slice of messages and the index, couldn't you just pass msgs[0]?
There was a problem hiding this comment.
Ah yes you're right, this can simplify things a little bit!
| func NewTxCmd() *cobra.Command { | ||
| cmd := &cobra.Command{ | ||
| Use: "host", | ||
| Short: "interchain-accounts host subcommands", |
There was a problem hiding this comment.
Suggested change
| Short: "interchain-accounts host subcommands", | |
| Short: "IBC interchain accounts host transaction subcommands", |
See this PR.
| into packet data which is outputted to stdout. It can be used in conjunction with send-tx" | ||
| which submits pre-built packet data containing messages to be executed on the host chain. | ||
| `, | ||
| Example: `<binary> tx interchain-accounts host generate-packet-data '{ |
There was a problem hiding this comment.
Can we use version.AppName instead of <binary> like in here?
| Short: "Generates ICA packet data.", | ||
| Long: `generate-packet-data accepts a message string and serializes it | ||
| into packet data which is outputted to stdout. It can be used in conjunction with send-tx" | ||
| which submits pre-built packet data containing messages to be executed on the host chain. |
Co-authored-by: Carlos Rodriguez <[email protected]>
…rate-ica-packet-data
…rate-ica-packet-data
chatton
deleted the
cian/issue#2243-add-a-ics27-host-cli-to-generate-ica-packet-data
branch
mergify bot
pushed a commit
that referenced
this pull request
Sep 28, 2022
(cherry picked from commit 8f0e7d5) # Conflicts: # CHANGELOG.md
colin-axner
added a commit
that referenced
this pull request
Sep 28, 2022
* Add host cli to generate ica packet data (#2297) (cherry picked from commit 8f0e7d5) # Conflicts: # CHANGELOG.md * fix conflict Co-authored-by: Cian Hatton <[email protected]> Co-authored-by: Carlos Rodriguez <[email protected]> Co-authored-by: colin axnér <[email protected]>
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.
Description
This PR creates a new cli command which generates ics27 packet data. I opted to display the packet data to stdout rather than writing to a file as I believe this is more flexible. In order to create a file we simply need
simd tx host generate-packet-data ... > packet-data.jsonNote: this is a
txcommand although it is a bit of an outlier as no tx is being broadcast. There is no good place that I can find to put this unique type of cli command so I stuck with thetxsubcommand.closes: #2243
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.Unreleasedsection inCHANGELOG.mdFiles changedin the Github PR explorerCodecov Reportin the comment section below once CI passes