docs: update transfer msg docs with ics20-2 fields #6504
Conversation
WalkthroughThe recent updates focus on modifying the Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant User
participant MsgTransfer
participant EventCallbacks
User->>MsgTransfer: Initiate Transfer with Tokens
MsgTransfer->>EventCallbacks: Trigger OnRecvPacket
EventCallbacks->>MsgTransfer: Process voucherDenom, ackSuccess, error
MsgTransfer->>User: Acknowledge Transfer Completion
sequenceDiagram
participant User
participant MsgTransfer
participant EventCallbacks
User->>MsgTransfer: Initiate Transfer with Tokens
MsgTransfer->>EventCallbacks: Trigger OnAcknowledgePacket
EventCallbacks->>MsgTransfer: Process acknowledgement
MsgTransfer->>User: Confirm Acknowledgement
sequenceDiagram
participant User
participant MsgTransfer
participant EventCallbacks
User->>MsgTransfer: Initiate Transfer with Tokens
MsgTransfer->>EventCallbacks: Trigger OnTimeoutPacket
EventCallbacks->>MsgTransfer: Process refund_tokens
MsgTransfer->>User: Notify Timeout and Refund
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 (
|
damiannolan
requested review from
colin-axner,
AdityaSripal,
crodriguezvega,
chatton,
DimitrisJim and
srdtrk
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (2)
docs/docs/02-apps/01-transfer/04-messages.md (2)
18-18: Clarify the deprecation notice for better understanding.Consider enhancing the deprecation notice for the
Tokenfield to explicitly state the version or date when it will be removed or become unsupported. This can help developers plan their transition to the newTokensfield more effectively.
Line range hint
48-48: Clarify the structure of the JSON object in the memo field.It might be helpful to specify that the JSON object structure in the memo field can vary depending on the middleware used. This clarification can prevent confusion and ensure proper implementation:
- The memo field should be represented as a json object where different middlewares use different json keys. + The memo field should be represented as a JSON object. The structure of this object may vary depending on the middleware used, with different middlewares requiring different JSON keys.Tools
LanguageTool
[style] ~33-~33: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym. (ENGLISH_WORD_REPEAT_BEGINNING_RULE)
Context: ...hs-identifiers-separators)). -Tokensis empty or aCoincontained within the ...
[uncategorized] ~40-~40: A comma may be missing after the conjunctive/linking adverb ‘Similarly’. (SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
Context: ...nis used thenTokensmust be empty. Similarly ifTokensis used thenToken` should...Markdownlint
34-34: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
35-35: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- docs/docs/02-apps/01-transfer/04-messages.md (1 hunks)
Additional context used
Path-based instructions (1)
docs/docs/02-apps/01-transfer/04-messages.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
LanguageTool
docs/docs/02-apps/01-transfer/04-messages.md
[style] ~33-~33: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym. (ENGLISH_WORD_REPEAT_BEGINNING_RULE)
Context: ...hs-identifiers-separators)). -Tokensis empty or aCoincontained within the ...
[uncategorized] ~40-~40: A comma may be missing after the conjunctive/linking adverb ‘Similarly’. (SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
Context: ...nis used thenTokensmust be empty. Similarly ifTokensis used thenToken` should...
[uncategorized] ~48-~48: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA)
Context: ...o field should be represented as a json object where different middlewares use differe...
Markdownlint
docs/docs/02-apps/01-transfer/04-messages.md
34-34: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
35-35: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
8-8: null (MD025, single-title, single-h1)
Multiple top-level headings in the same document
Additional comments not posted (1)
docs/docs/02-apps/01-transfer/04-messages.md (1)
25-25: Ensure the newTokensfield is correctly documented.The addition of the
Tokensfield is well-documented. Make sure to update any related code examples or integration tests to reflect this change.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (1)
docs/docs/02-apps/01-transfer/04-messages.md (1)
Line range hint
8-8: Address multiple top-level headings in the document.The document should not have multiple top-level headings (
#). This can cause issues with document structure and navigation:- # Messages + ## MessagesTools
LanguageTool
[uncategorized] ~41-~41: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA)
Context: ...now useTokensinstead. IfTokenis used thenTokensmust be empty. Similarly,...
[uncategorized] ~41-~41: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA)
Context: ...ust be empty. Similarly, ifTokensis used thenTokenshould be left empty. This...
[uncategorized] ~42-~42: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA)
Context: ...nd a fungible token to the counterparty chain represented by the counterparty Channel...Markdownlint
35-35: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
36-36: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- docs/docs/02-apps/01-transfer/04-messages.md (1 hunks)
Additional context used
Path-based instructions (1)
docs/docs/02-apps/01-transfer/04-messages.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
LanguageTool
docs/docs/02-apps/01-transfer/04-messages.md
[uncategorized] ~41-~41: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA)
Context: ...now useTokensinstead. IfTokenis used thenTokensmust be empty. Similarly,...
[uncategorized] ~41-~41: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA)
Context: ...ust be empty. Similarly, ifTokensis used thenTokenshould be left empty. This...
[uncategorized] ~42-~42: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA)
Context: ...nd a fungible token to the counterparty chain represented by the counterparty Channel...
[uncategorized] ~48-~48: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA)
Context: ...o field should be represented as a json object where different middlewares use differe...
Markdownlint
docs/docs/02-apps/01-transfer/04-messages.md
35-35: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
36-36: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
8-8: null (MD025, single-title, single-h1)
Multiple top-level headings in the same document
Additional comments not posted (1)
docs/docs/02-apps/01-transfer/04-messages.md (1)
18-18: Update theMsgTransferstruct to reflect the deprecation ofTokenand addition ofTokens.The changes correctly reflect the deprecation of the
Tokenfield and the addition of theTokensfield as per the PR objectives. This update aligns with the ICS20-2 specifications.Also applies to: 25-25
| - `Amount` must be positive. | ||
| - `Denom` must be a valid IBC denomination, as defined in [ADR 001 - Coin Source Tracing](/architecture/adr-001-coin-source-tracing). |
There was a problem hiding this comment.
Correct indentation in markdown list.
The indentation for the list items under Tokens validation rules is inconsistent with markdown standards. Here's a corrected version:
- - `Amount` must be positive.
- - `Denom` must be a valid IBC denomination, as defined in [ADR 001 - Coin Source Tracing](/architecture/adr-001-coin-source-tracing).
+ - `Amount` must be positive.
+ - `Denom` must be a valid IBC denomination, as defined in [ADR 001 - Coin Source Tracing](/architecture/adr-001-coin-source-tracing).Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| - `Amount` must be positive. | |
| - `Denom` must be a valid IBC denomination, as defined in [ADR 001 - Coin Source Tracing](/architecture/adr-001-coin-source-tracing). | |
| - `Amount` must be positive. | |
| - `Denom` must be a valid IBC denomination, as defined in [ADR 001 - Coin Source Tracing](/architecture/adr-001-coin-source-tracing). |
Tools
Markdownlint
35-35: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
36-36: Expected: 2; Actual: 4 (MD007, ul-indent)
Unordered list indentation
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (1)
docs/docs/02-apps/01-transfer/05-events.md (1)
8-8: Address the issue of multiple top-level headings in the document.Consider restructuring the document to have a single top-level heading or adjust the heading levels appropriately.
Tools
Markdownlint
8-8: null (MD025, single-title, single-h1)
Multiple top-level headings in the same document
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- docs/docs/02-apps/01-transfer/05-events.md (2 hunks)
Additional context used
Path-based instructions (1)
docs/docs/02-apps/01-transfer/05-events.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
Markdownlint
docs/docs/02-apps/01-transfer/05-events.md
24-24: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces)
Trailing spaces
25-25: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces)
Trailing spaces
29-29: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces)
Trailing spaces
8-8: null (MD025, single-title, single-h1)
Multiple top-level headings in the same document
Additional comments not posted (4)
docs/docs/02-apps/01-transfer/05-events.md (4)
16-16: Ensure the newtokensattribute is correctly documented and matches the updatedMsgTransferstruct.
22-32: Review the updated attributes for theOnRecvPacketcallback.Tools
Markdownlint
24-24: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces)
Trailing spaces
25-25: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces)
Trailing spaces
29-29: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces)
Trailing spaces
36-44: Review the updated attributes for theOnAcknowledgePacketcallback.
48-53: Review the updated attributes for theOnTimeoutPacketcallback.
| | denomination | denom | \{denom\} | | ||
| | Type | Attribute Key | Attribute Value | | ||
| |-----------------------|---------------|------------------| | ||
| | fungible_token_packet | sender | \{sender\} | |
There was a problem hiding this comment.
Remove trailing spaces to adhere to Markdown best practices.
- | fungible_token_packet | sender | \{sender\} |
+ | fungible_token_packet | sender | \{sender\} |
- | fungible_token_packet | receiver | \{receiver\} |
+ | fungible_token_packet | receiver | \{receiver\} |
- | fungible_token_packet | memo | \{memo\} |
+ | fungible_token_packet | memo | \{memo\} |Also applies to: 25-25, 29-29
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| | fungible_token_packet | sender | \{sender\} | | |
| | fungible_token_packet | sender | \{sender\} | | |
| | fungible_token_packet | receiver | \{receiver\} | | |
| | fungible_token_packet | memo | \{memo\} | |
Tools
Markdownlint
24-24: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces)
Trailing spaces
| @@ -14,42 +13,41 @@ slug: /apps/transfer/events | |||
| |--------------|---------------|-----------------| | |||
| | ibc_transfer | sender | \{sender\} | | |||
| | ibc_transfer | receiver | \{receiver\} | | |||
| | message | action | transfer | | |||
There was a problem hiding this comment.
I updated fields like this that were not change now, but were incorrect from before. Should we update them for older versions as well?
There was a problem hiding this comment.
Yes that would be great 👍
There was a problem hiding this comment.
Created a separate issue for this: #6512
There was a problem hiding this comment.
it also doesn't seem we document Denom events?
There was a problem hiding this comment.
They are documented under OnRecvPacket, which is the only place we emit them, I believe
There was a problem hiding this comment.
ah, missed! should probs be updated too then, we emit the Denom as a json too #6467 (comment)
Description
closes: #6389
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/).godoccomments.Files changedin the GitHub PR explorer.SonarCloud Reportin the comment section below once CI passes.Summary by CodeRabbit
Tokenfield inMsgTransferand replaced it withTokens, a slice ofsdk.Coin. Updated validation rules and usage guidelines accordingly.MsgTransferevents, affecting behavior and data associated with these events.