fix: ibc transfer memo and receiver length check (#2551) #2553
Conversation
WalkthroughRecent changes primarily address bug fixes and updates across multiple files. Key modifications include restricting the length of IBC transfer memo and receiver fields, transitioning upgrade registrations from Changes
Poem
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 (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2553 +/- ##
===========================================
- Coverage 75.38% 62.77% -12.62%
===========================================
Files 100 275 +175
Lines 8025 15969 +7944
===========================================
+ Hits 6050 10024 +3974
- Misses 1589 5171 +3582
- Partials 386 774 +388
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Outside diff range and nitpick comments (1)
tests/tsdk/string.go (1)
10-10: Specify the lint rule being ignored to clarify the intent and ensure only specific issues are overlooked.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- CHANGELOG.md (2 hunks)
- README.md (1 hunks)
- RELEASE_NOTES.md (2 hunks)
- app/upgrades.go (1 hunks)
- tests/e2e/e2e_ibc_test.go (2 hunks)
- tests/tsdk/string.go (1 hunks)
- tests/tsdk/string_test.go (1 hunks)
- util/ibc/ibc_test.go (2 hunks)
Additional context used
LanguageTool
RELEASE_NOTES.md
[grammar] ~30-~30: The auxiliary verb ‘do’ requires the base form of the verb. (DID_BASEFORM)
Context: ...ved that Price Feeder doesn't correctly re-established a connection after the chain upgrade. ...README.md
[uncategorized] ~20-~20: The adjective “market-driven” is spelled with a hyphen. (DRIVEN_HYPHEN)
Context: ...ally determined interest rates based on market driven conditions. As a cross chain DeFi proto...
[grammar] ~26-~26: You’ve repeated a verb. Did you mean to only write one of them? (REPEATED_VERBS)
Context: ...ble of Contents](#table-of-contents) - Releases - [Release Compatibility Matrix](#release-compatib...
[uncategorized] ~76-~76: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA)
Context: ...ld the binary from source on the server machine you probably don't need any change. Bui...
[uncategorized] ~77-~77: A comma may be missing after the conjunctive/linking adverb ‘However’. (SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
Context: ...reated as a part of the build process. However when you download a binary from GitHub,...
[grammar] ~114-~114: Did you mean “installing”? Or maybe you should add a pronoun? In active voice, ‘require’ + ‘to’ takes an object, usually a pronoun. (ALLOW_TO)
Context: ...e recommend to use RocksDB. It requires to installrocksdbsystem libraries. We plan to ...
[grammar] ~115-~115: An article may be missing. (BUNCH_OF)
Context: ...newer version of badgerdb, which brings lot of improvements and simplifies the setup. ...CHANGELOG.md
[grammar] ~133-~133: Using ‘plenty’ without ‘of’ is considered to be informal. (PLENTY_OF_NOUNS)
Context: .../pull/2368) Fix inflow amount calculation. Previously, the inflow amount of the t...
[grammar] ~235-~235: Did you mean “limiting”? Or maybe you should add a pronoun? In active voice, ‘allow’ + ‘to’ takes an object, usually a pronoun. (ALLOW_TO)
Context: ...veragedLiquidate.MaxRepay` which allows to limit the liquidation size using the leverage...
[grammar] ~368-~368: The singular proper name ‘Bridge’ must be used with a third-person or a past tense verb. (HE_VERB_AGR)
Context: ...-network/umee/pull/1967) Gravity Bridge phase out phase-2: disable Umee -> Ethereum t...
[grammar] ~369-~369: The singular proper name ‘Bridge’ must be used with a third-person or a past tense verb. (HE_VERB_AGR)
Context: ...-network/umee/pull/1967) Gravity Bridge phase out phase-2: disable Umee -> Ethereum t...
[grammar] ~419-~419: Make sure that the singular noun after the number ‘4.1’ is correct. (CD_POINT_CD_NN)
Context: ...e/pull/1807) Fixes BNB ibc denom in 4.1 migration - [1812](https://github.com/umee-networ...
[style] ~437-~437: In contexts where ‘if’ is followed by ‘or’, using ‘whether’ may be more appropriate (and formal). (IF_WHETHER)
Context: ...rowandMsgRepay` won't return errors if there is nothing to withdraw, borrow or...
[duplication] ~488-~488: Possible typo: you repeated a word (ENGLISH_WORD_REPEAT_RULE)
Context: ...e/pull/1555) Updates IBC to v5.1.0 that adds adds optional memo field to `FungibleTokenPa...
[uncategorized] ~545-~545: A comma may be missing after the conjunctive/linking adverb ‘Also’. (SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
Context: ...ableLend, docs, and internal functions. Also QueryLoaned similar queries to QuerySup...
[misspelling] ~549-~549: This word is normally spelled as one. (EN_COMPOUNDS_PROTO_TYPES)
Context: ...roto getters in x/leverage and x/oracle proto types. - [1126](https://github.com/umee-netwo...
[duplication] ~668-~668: Possible typo: you repeated a word (ENGLISH_WORD_REPEAT_RULE)
Context: ...k/umee/pull/1358/files) Disable Gravity Bridge bridge messages. ### Improvements - [#1355](...
Additional comments not posted (7)
util/ibc/ibc_test.go (2)
Line range hint
59-67: This test appears to be straightforward and correctly verifies the functionality ofGetLocalDenom.
38-52: Ensure that the error messages for exceeding memo and receiver lengths are descriptive and actionable.Verification successful
The error messages for exceeding memo and receiver lengths are correctly implemented and verified in the codebase.
util/ibc/ibc.go: The error messages for "recipient address must not exceed" and "memo must not exceed" are defined and used appropriately.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Ensure that the error messages are accurate and provide enough information for debugging. grep -rn 'must not exceed' util/ibc/ibc_test.goLength of output: 176
Script:
#!/bin/bash # Locate the functions that generate the error messages for exceeding memo and receiver lengths. ast-grep --lang go --pattern 'return fmt.Errorf("recipient address must not exceed")' ast-grep --lang go --pattern 'return fmt.Errorf("memo must not exceed")'Length of output: 162
Script:
#!/bin/bash # Search for the error message strings in the codebase to identify their definitions or usage. rg 'recipient address must not exceed' rg 'memo must not exceed'Length of output: 447
README.md (1)
68-68: Ensure that all dependencies and compatibility references are updated to reflect the changes in versionv6.6.x.Verification successful
The version
v6.6.xis only referenced in theREADME.mdfile. This indicates that the update is localized and doesn't impact other parts of the codebase.
- README.md: Line 68
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all compatibility references for version `v6.6.x`. # Test: Search for version `v6.6.x` in project files. Expect: Consistent versioning across all files. rg --type md 'v6.6.x'Length of output: 148
Script:
#!/bin/bash # Description: Verify all compatibility references for version `v6.6.x` across all project files. # Test: Search for version `v6.6.x` in all project files. Expect: Consistent versioning across all files. rg 'v6.6.x'Length of output: 138
app/upgrades.go (1)
57-57: The addition of the upgrade handler forv6.6is well-implemented. Ensure that all related modules and dependencies are correctly updated to support this version.tests/e2e/e2e_ibc_test.go (2)
14-14: The update of the import statement fromsdkutiltotsdkis correctly implemented. Ensure that all references and usages in the test cases are updated accordingly.
169-171: The addition of the test for generating a string with a length greater than the maximum allowed is a good practice to ensure robustness. Ensure that the test covers all edge cases.CHANGELOG.md (1)
51-51: Ensure accuracy in the documentation of PR links and descriptions.This entry correctly links to PR 2551 and describes the change made to restrict the length of IBC transfer memo and receiver fields, aligning with the PR objectives.
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (2)
RELEASE_NOTES.md (2)
21-22: The description of the new auction module is clear and concise. However, consider adding a brief overview of how this module affects existing users or interacts with other parts of the system for a more comprehensive understanding.
Line range hint
34-34: Consider adding a comma after "server machine" for better readability.- on the server machine you probably don't need any change + on the server machine, you probably don't need any change
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- CHANGELOG.md (1 hunks)
- RELEASE_NOTES.md (2 hunks)
Additional context used
LanguageTool
RELEASE_NOTES.md
[uncategorized] ~34-~34: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA)
Context: ...ld the binary from source on the server machine you probably don't need any change. How...CHANGELOG.md
[grammar] ~129-~129: Using ‘plenty’ without ‘of’ is considered to be informal. (PLENTY_OF_NOUNS)
Context: .../pull/2368) Fix inflow amount calculation. Previously, the inflow amount of the t...
[grammar] ~231-~231: Did you mean “limiting”? Or maybe you should add a pronoun? In active voice, ‘allow’ + ‘to’ takes an object, usually a pronoun. (ALLOW_TO)
Context: ...veragedLiquidate.MaxRepay` which allows to limit the liquidation size using the leverage...
[grammar] ~364-~364: The singular proper name ‘Bridge’ must be used with a third-person or a past tense verb. (HE_VERB_AGR)
Context: ...-network/umee/pull/1967) Gravity Bridge phase out phase-2: disable Umee -> Ethereum t...
[grammar] ~365-~365: The singular proper name ‘Bridge’ must be used with a third-person or a past tense verb. (HE_VERB_AGR)
Context: ...-network/umee/pull/1967) Gravity Bridge phase out phase-2: disable Umee -> Ethereum t...
[grammar] ~415-~415: Make sure that the singular noun after the number ‘4.1’ is correct. (CD_POINT_CD_NN)
Context: ...e/pull/1807) Fixes BNB ibc denom in 4.1 migration - [1812](https://github.com/umee-networ...
[style] ~433-~433: In contexts where ‘if’ is followed by ‘or’, using ‘whether’ may be more appropriate (and formal). (IF_WHETHER)
Context: ...rowandMsgRepay` won't return errors if there is nothing to withdraw, borrow or...
[duplication] ~484-~484: Possible typo: you repeated a word (ENGLISH_WORD_REPEAT_RULE)
Context: ...e/pull/1555) Updates IBC to v5.1.0 that adds adds optional memo field to `FungibleTokenPa...
[uncategorized] ~541-~541: A comma may be missing after the conjunctive/linking adverb ‘Also’. (SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
Context: ...ableLend, docs, and internal functions. Also QueryLoaned similar queries to QuerySup...
[misspelling] ~545-~545: This word is normally spelled as one. (EN_COMPOUNDS_PROTO_TYPES)
Context: ...roto getters in x/leverage and x/oracle proto types. - [1126](https://github.com/umee-netwo...
[duplication] ~664-~664: Possible typo: you repeated a word (ENGLISH_WORD_REPEAT_RULE)
Context: ...k/umee/pull/1358/files) Disable Gravity Bridge bridge messages. ### Improvements - [#1355](...
Additional comments not posted (3)
RELEASE_NOTES.md (2)
9-9: LGTM! Clear summary of the new version highlights.
26-26: The upgrade title is correctly updated to reflect the new version.CHANGELOG.md (1)
73-73: Duplicate entry for IBC transfer memo and receiver length restriction.The same entry about restricting the length of IBC transfer memo and receiver fields appears under both the
Unreleasedandv6.5.0sections. This duplication should be removed from one of the sections to maintain clarity and avoid redundancy in the changelog.
Description
Summary by CodeRabbit
New Features
GenerateStringfunction in the newtsdkpackage for generating random strings.Bug Fixes
x/ugovmessages.MissCountersgrpc-web get API result.Documentation
Refactor
tsdkpackage for test utilities.