-
Notifications
You must be signed in to change notification settings - Fork 580
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[FEAT] ICS20-V2 #6352
[FEAT] ICS20-V2 #6352
Conversation
* chore: adding proto files for ics20-v2 * chore: add newline
* add CurrentVersion, EscrowVersion, update tests * update hardcoded transfer channel version from interchaintest
--------- Co-authored-by: Charly <charly@interchain.berlin>
…shalling/conversion (#6226) * chore: adding proto files for ics20-v2 * chore: add newline * chore: modify MsgTransfer to accept coins instead of coin * chore: reverted unintentional comment changes * chore: reverted unintentional comment changes * chore: adding test for conversion fn * chore: fix e2e linter * chore: adding docs * chore: addressing PR feedback * chore: remove duplicate import * chore: addressing PR feedback * chore: moved extration logic into internal package * chore: commented out failing test * chore: adding link to issue * chore: remove duplicate import * chore: fix linting errors * FungibleTokenPacketData interface methods + tests * linter * wip: token validation * update trace identifier validation in Token + tests * rm Printf * update with pr review * add CurrentVersion, EscrowVersion, update tests * pr review * fix e2e tests * pr review * update e2e test version * linter * update hardcoded transfer channel version from interchaintest * update hardcoded transfer channel version from interchaintest * wip packet unmarshaller * wip * wip testing * update test * linter * rm unnecessary version changes * rm unnecessary artifacts * update callbacks test * update comment * pr review * rename getMultiDenomFungibleTokenPacketData to unmarshalPacketDataBytesToICS20V2 --------- Co-authored-by: chatton <github.qpeyb@simplelogin.fr> Co-authored-by: Carlos Rodriguez <carlos@interchain.io>
…allbacks (#6175) * add SupportedVersions array for different ics20 versions, add version checking on channel handshake application callbacks * add tests * update pr review * pr review * last few pr review nits * linter * add version counter proposing * fix missing app versino * update code + tests to return our proposed version if counterparty version is invalid * remove if statement * address review comments: return ics20-2 if counterparty version is not supported --------- Co-authored-by: Carlos Rodriguez <carlos@interchain.io>
…transfers (#6252) * add transfer authz code + tests * linter * address review comments --------- Co-authored-by: Carlos Rodriguez <carlos@interchain.io>
…ks (#6189) * chore: adding proto files for ics20-v2 * chore: add newline * chore: modify MsgTransfer to accept coins instead of coin * chore: reverted unintentional comment changes * chore: reverted unintentional comment changes * chore: adding test for conversion fn * chore: fix e2e linter * chore: adding docs * chore: addressing PR feedback * chore: remove duplicate import * chore: addressing PR feedback * chore: moved extration logic into internal package * chore: commented out failing test * chore: adding link to issue * chore: remove duplicate import * chore: fix linting errors * FungibleTokenPacketData interface methods + tests * linter * wip: token validation * update trace identifier validation in Token + tests * rm Printf * update with pr review * pr review * linter * rm unused function: linter * wip pr feedback * fix test * pr review * lintttttt * wip: backwards compatibility for transfer rpc * implement changes for ics20-v2 packet data for onRecvPacket, onAcknowledgePacket and onTimeoutPacket * fix callbacks tests * lint --------- Co-authored-by: chatton <github.qpeyb@simplelogin.fr> Co-authored-by: Charly <charly@interchain.berlin>
Sync feature with main branch
WalkthroughWalkthroughThe changes primarily introduce support for multi-denomination transfers in the IBC (Inter-Blockchain Communication) module. This includes updating the Changes
Sequence Diagram(s) (Beta)No sequence diagrams are provided as the changes are too varied and do not represent a single control flow. Assessment against linked issues
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 (
|
* refactor: quick change to tokens event attribute * fix: various tests * lint * lint:fixeroni * imp: remove events variable in favour of direct event emission --------- Co-authored-by: DimitrisJim <d.f.hilliard@gmail.com>
@@ -49,6 +49,8 @@ message MsgTransfer { | |||
uint64 timeout_timestamp = 7; | |||
// optional memo | |||
string memo = 8; | |||
// tokens to be transferred | |||
repeated cosmos.base.v1beta1.Coin tokens = 9 [(gogoproto.nullable) = false, (amino.dont_omitempty) = true]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we deprecate the token
field?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea. is the amino
tag necessary anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it could be removed on the token
potentially. This would need to be tested with ledger signing for compatability. If we deprecate and leave the tag, an empty Coin
would always be displayed on ledger screen before signing (don't omit empty) but if we remove the tag and the Coin
is empty because the new repeated field tokens
is being used then I would imagine the empty coin is omitted and not displayed on the ledger screen (which I think is better ux).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aside from above. The repeated field needs afaik: (amino.encoding) = "legacy_coins",
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sdk also uses (gogoproto.castrepeated) = "github.com/cosmos/cosmos-sdk/types.Coins"
for fields like this, maybe we should also be doing that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docs on that https://github.com/cosmos/gogoproto/blob/main/extensions.md#more-canonical-go-structures
we can ask if its needed? Currently, adding castrepeated
generates Token
as:
Tokens github_com_cosmos_cosmos_sdk_types.Coins `protobuf:"bytes,9,rep,name=tokens,proto3,castrepeated=github.com/cosmos/cosmos-sdk/types.Coins" json:"tokens"`
with github_com_cosmos_cosmos_sdk_types "github.com/cosmos/cosmos-sdk/types"
as an ungodly import name.
can't see much else in the diff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
raised this in slack with sdk.
* check length of tokens array against maximum allowed * chore: add note on arbitrary selection --------- Co-authored-by: Colin Axnér <25233464+colin-axner@users.noreply.github.com>
* api(port)!: Allow passing of context, port and channel identifier to unmarshal packet data interface as disussed. This allows us to grab the app version in transfer and unmarshal the packet based on that instead of a hacky unmarshal v2 then v1 and whatever happens. * lint: as we do * callbacks: fix signature of UnmarshalPacketData as per changes, make refactors to hopefully simplify signatures. * chore: lint and remove some todos. * review: address feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
leaving first part of review, will pick up rest tomorrow 🫡
@@ -49,6 +49,8 @@ message MsgTransfer { | |||
uint64 timeout_timestamp = 7; | |||
// optional memo | |||
string memo = 8; | |||
// tokens to be transferred | |||
repeated cosmos.base.v1beta1.Coin tokens = 9 [(gogoproto.nullable) = false, (amino.dont_omitempty) = true]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea. is the amino
tag necessary anymore?
* chore: specifically unmarshal v1 or v2 without brute force * chore: fix TestPacketDataUnmarshalerInterface test in transfer module * Pass dest values OnRecv, refactor GetExpectedEvents * chore: fixing TestGetCallbackData test * chore: fixed remaining tests in callbacks module * test: simplify callbacks test, revert back to previous behaviour * chore: fix test case name * chore: addressing PR feedback * chore: added docstring for unmarshalPacketDataBytesToICS20V2 --------- Co-authored-by: DimitrisJim <d.f.hilliard@gmail.com> Co-authored-by: Colin Axnér <25233464+colin-axner@users.noreply.github.com>
Sync Feature Branch With Main
|
||
chainA, chainB *ibctesting.TestChain | ||
|
||
path *ibctesting.Path | ||
} | ||
|
||
// SetupTest creates a coordinator with 1 test chain. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment needs to be updated
if err := packetData.ValidateBasic(); err != nil { | ||
panic(err) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
think this panic was a hack from the PoC, I think we can probably return an error from this fn in the case of invalid packet data.
|
||
// this condition should never be reached. | ||
if len(splitPath)%2 != 0 { | ||
panic("pathSlice length is not even") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
though unreachable, panic with err for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes! I had another panic cleanup related concern, will create an issue to clean these up.
@@ -49,6 +49,8 @@ message MsgTransfer { | |||
uint64 timeout_timestamp = 7; | |||
// optional memo | |||
string memo = 8; | |||
// tokens to be transferred | |||
repeated cosmos.base.v1beta1.Coin tokens = 9 [(gogoproto.nullable) = false, (amino.dont_omitempty) = true]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docs on that https://github.com/cosmos/gogoproto/blob/main/extensions.md#more-canonical-go-structures
we can ask if its needed? Currently, adding castrepeated
generates Token
as:
Tokens github_com_cosmos_cosmos_sdk_types.Coins `protobuf:"bytes,9,rep,name=tokens,proto3,castrepeated=github.com/cosmos/cosmos-sdk/types.Coins" json:"tokens"`
with github_com_cosmos_cosmos_sdk_types "github.com/cosmos/cosmos-sdk/types"
as an ungodly import name.
can't see much else in the diff.
* refactor: address various self review comments * revert: unnecessary change * lint
* refactor: apply review suggestions * imp: additional updates * refactor: make ValidateIBCDenom private * Update modules/apps/transfer/types/msgs.go Co-authored-by: Cian Hatton <cian@interchain.io> * apply review suggestions --------- Co-authored-by: Cian Hatton <cian@interchain.io>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there, I got 10 files left!
* chore: move functions from internal/denom to trace.go * lint * lint: the comeback
} | ||
|
||
limitLeft, isNegative := allocation.SpendLimit.SafeSub(msgTransfer.Token) | ||
limitLeft, isNegative := a.Allocations[index].SpendLimit.SafeSub(coin) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
limitLeft, isNegative := a.Allocations[index].SpendLimit.SafeSub(coin) | |
limitLeft, isNegative := allocation.SpendLimit.SafeSub(coin) |
take it or leave it, just felt odd relative to line 74
|
||
if tc.expPass { | ||
var expEvents []abci.Event | ||
if tc.multiDenom { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could probably simplify by just expecting the event attribute to be msg.Tokens
} | ||
} | ||
|
||
// extractDenomAndTraceFromV1Denom extracts the base denom and remaining trace from a v1 IBC denom. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// extractDenomAndTraceFromV1Denom extracts the base denom and remaining trace from a v1 IBC denom. | |
// ExtractDenomAndTraceFromV1Denom extracts the base denom and remaining trace from a v1 IBC denom. |
looks like it wasn't made private?
} | ||
|
||
// extractDenomAndTraceFromV1Denom extracts the base denom and remaining trace from a v1 IBC denom. | ||
func ExtractDenomAndTraceFromV1Denom(v1Denom string) (string, []string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this function appears to assume the denom is validated, should we add this in the godoc?
Otherwise the parsing logic doesn't account for edge cases like the other function does
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* imp: self review items * apply jim's suggestion * Update modules/apps/transfer/keeper/msg_server_test.go * Update modules/apps/transfer/ibc_module.go * Update modules/apps/transfer/ibc_module.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alrighty! Wahoo! I approve! Finally made it to the end 🚀
Many kudos and excellent work by everyone who contributed to this feature! 👏 👏 👏
The feature is clean and gives transfer that much needed face lift. I can feel it sets a nice tone to remove some unnecessary complexity under the hood.
There are a few requested changes we should do after merge to main:
- Migrate internal storage of trace string to array of trace info #6221 (I think this will help clean up a lot of logic)
- Use typed Trace in ics20 v2 Token type #6362
- create transfer packet data type based on channel version #6376 (necessary for correctness)
// retrieved on the step above and it has enough balance | ||
// to burn. | ||
panic(fmt.Errorf("cannot burn coins after a successful send to a module account: %v", err)) | ||
denom, trace := convertinternal.ExtractDenomAndTraceFromV1Denom(fullDenomPath) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think when we do the check if the coin.Denom has the "ibc" prefix, we should either set the trace that's found upon lookup, or set it to nil (native coin)
That would remove this extra step of parsing (following #6221). We should have all the info in state, no need to construct the path and reparse it. Upon sending, we should be using v2 logic only (unless we need to send as v1 packet)
labels := []metrics.Label{ | ||
telemetry.NewLabel(coretypes.LabelSourcePort, packet.GetSourcePort()), | ||
telemetry.NewLabel(coretypes.LabelSourceChannel, packet.GetSourceChannel()), | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can be set once? not per token?
// NOTE: We use SourcePort and SourceChannel here, because the counterparty | ||
// chain would have prefixed with DestPort and DestChannel when originally | ||
// receiving this coin as seen in the "sender chain is the source" condition. | ||
if types.ReceiverChainIsSource(packet.GetSourcePort(), packet.GetSourceChannel(), fullDenomPath) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these funcs probably can be updated to look at the trace type (maybe could add as func to token)
telemetry.SetGaugeWithLabels( | ||
[]string{"ibc", types.ModuleName, "packet", "receive"}, | ||
float32(transferAmount.Int64()), | ||
[]metrics.Label{telemetry.NewLabel(coretypes.LabelDenom, unprefixedDenom)}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably should have been based on the coin.Denom, not the packet representation
if !ok { | ||
return errorsmod.Wrapf(types.ErrInvalidAmount, "unable to parse transfer amount (%s) into math.Int", transferAmount) | ||
} | ||
token := sdk.NewCoin(trace.IBCDenom(), transferAmount) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe lets rename to coin
given that there's another variable here also named token
with a different type and denom value 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#6379 opened this, there might some additional places where these are used somewhat interchangeably and would be nice to grab em in one swoop.
* chore: refactoring msgs_test.go to use expError * chore: updating expected errors * chore: update MsgUpdateParams and lint --------- Co-authored-by: DimitrisJim <d.f.hilliard@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 18
Outside diff range and nitpick comments (9)
modules/apps/transfer/types/trace.go (2)
Line range hint
87-114
: Refactor to improve clarity and robustness inextractPathAndBaseFromFullDenom
.- for i := 0; i < length; i += 2 { - if i < length-1 && length > 2 && channeltypes.IsValidChannelID(fullDenomItems[i+1]) { - pathSlice = append(pathSlice, fullDenomItems[i], fullDenomItems[i+1]) - } else { - baseDenomSlice = fullDenomItems[i:] - break - } - } + for i := 0; i < length-1; i += 2 { + if channeltypes.IsValidChannelID(fullDenomItems[i+1]) { + pathSlice = append(pathSlice, fullDenomItems[i], fullDenomItems[i+1]) + } else { + baseDenomSlice = fullDenomItems[i:] + break + } + }This change ensures that the loop does not unnecessarily check the length condition in each iteration and clarifies the conditions under which the loop breaks.
202-207
: Clarify the comment regarding base denomination validation inValidatePrefixedDenom
.Consider adding more detail to the comment about why no base denomination validation is performed when the path is empty, to help future maintainers understand the decision.
modules/apps/callbacks/types/callbacks_test.go (1)
302-305
: Ensure consistency in IBC channel configurations.The channel configurations for tests are hardcoded to use
transfertypes.V1
. Given the PR's focus on transitioning to V2, ensure that these tests are either updated or additional tests are added to cover the V2 configurations.Also applies to: 347-350
modules/apps/callbacks/ibc_middleware_test.go (3)
Line range hint
96-203
: Consider adding more detailed comments explaining the test scenarios, especially for complex cases.Adding comments can improve maintainability and make it easier for new contributors to understand the purpose and expected behavior of each test case.
Line range hint
403-546
: TheTestOnTimeoutPacket
function should include a test case for the scenario where the timeout is exactly on the boundary condition.Adding a boundary condition test can help ensure robustness, especially in timing-sensitive operations like IBC packet timeouts.
Line range hint
974-1052
: The tests forUnmarshalPacketDataV1
andUnmarshalPacketDataV2
should include scenarios where packet data is corrupted or incomplete.Consider adding negative test cases to cover scenarios of corrupted or incomplete packet data. This can help ensure the robustness of the unmarshaling logic.
CHANGELOG.md (3)
Line range hint
216-216
: Remove trailing spaces.- * (core/04-channel) [\#5788](https://github.com/cosmos/ibc-go/pull/5788) Add `NewErrorAcknowledgementWithCodespace` to allow codespaces in ack errors. + * (core/04-channel) [\#5788](https://github.com/cosmos/ibc-go/pull/5788) Add `NewErrorAcknowledgementWithCodespace` to allow codespaces in ack errors.
Line range hint
266-266
: Remove trailing spaces.- * (apps/27-interchain-accounts) [\#5633](https://github.com/cosmos/ibc-go/pull/5633) Allow new ICA channels to use unordered ordering. + * (apps/27-interchain-accounts) [\#5633](https://github.com/cosmos/ibc-go/pull/5633) Allow new ICA channels to use unordered ordering.
Line range hint
201-201
: Avoid using bare URLs. Provide a descriptive text for the hyperlink.- * [\#3133](https://github.com/cosmos/ibc-go/pull/3133) Add linter for markdown documents. + * [Add linter for markdown documents](https://github.com/cosmos/ibc-go/pull/3133).
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files ignored due to path filters (2)
modules/apps/transfer/types/packet.pb.go
is excluded by!**/*.pb.go
,!**/*.pb.go
modules/apps/transfer/types/tx.pb.go
is excluded by!**/*.pb.go
,!**/*.pb.go
Files selected for processing (57)
- CHANGELOG.md (1 hunks)
- docs/docs/05-migrations/13-v8-to-v9.md (1 hunks)
- e2e/tests/transfer/authz_test.go (4 hunks)
- e2e/tests/transfer/incentivized_test.go (2 hunks)
- e2e/tests/transfer/localhost_test.go (3 hunks)
- e2e/tests/transfer/upgrades_test.go (4 hunks)
- e2e/tests/upgrades/upgrade_test.go (1 hunks)
- e2e/testsuite/testsuite.go (3 hunks)
- e2e/testsuite/tx.go (1 hunks)
- e2e/testvalues/values.go (1 hunks)
- modules/apps/27-interchain-accounts/controller/ibc_middleware.go (1 hunks)
- modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go (1 hunks)
- modules/apps/27-interchain-accounts/host/ibc_module.go (1 hunks)
- modules/apps/27-interchain-accounts/host/ibc_module_test.go (1 hunks)
- modules/apps/27-interchain-accounts/host/keeper/relay_test.go (2 hunks)
- modules/apps/29-fee/ibc_middleware.go (1 hunks)
- modules/apps/29-fee/ibc_middleware_test.go (2 hunks)
- modules/apps/29-fee/keeper/events_test.go (2 hunks)
- modules/apps/29-fee/transfer_test.go (5 hunks)
- modules/apps/callbacks/callbacks_test.go (5 hunks)
- modules/apps/callbacks/ibc_middleware.go (6 hunks)
- modules/apps/callbacks/ibc_middleware_test.go (18 hunks)
- modules/apps/callbacks/replay_test.go (1 hunks)
- modules/apps/callbacks/transfer_test.go (2 hunks)
- modules/apps/callbacks/types/callbacks.go (2 hunks)
- modules/apps/callbacks/types/callbacks_test.go (14 hunks)
- modules/apps/callbacks/types/export_test.go (1 hunks)
- modules/apps/callbacks/types/types_test.go (1 hunks)
- modules/apps/transfer/client/cli/tx.go (3 hunks)
- modules/apps/transfer/ibc_module.go (13 hunks)
- modules/apps/transfer/ibc_module_test.go (11 hunks)
- modules/apps/transfer/internal/convert/convert.go (1 hunks)
- modules/apps/transfer/internal/convert/convert_test.go (1 hunks)
- modules/apps/transfer/keeper/invariants_test.go (1 hunks)
- modules/apps/transfer/keeper/mbt_relay_test.go (6 hunks)
- modules/apps/transfer/keeper/msg_server.go (2 hunks)
- modules/apps/transfer/keeper/msg_server_test.go (5 hunks)
- modules/apps/transfer/keeper/relay.go (6 hunks)
- modules/apps/transfer/keeper/relay_test.go (24 hunks)
- modules/apps/transfer/transfer_test.go (3 hunks)
- modules/apps/transfer/types/events.go (1 hunks)
- modules/apps/transfer/types/expected_keepers.go (1 hunks)
- modules/apps/transfer/types/export_test.go (1 hunks)
- modules/apps/transfer/types/keys.go (3 hunks)
- modules/apps/transfer/types/msgs.go (4 hunks)
- modules/apps/transfer/types/msgs_test.go (4 hunks)
- modules/apps/transfer/types/packet.go (1 hunks)
- modules/apps/transfer/types/packet_test.go (2 hunks)
- modules/apps/transfer/types/token.go (1 hunks)
- modules/apps/transfer/types/token_test.go (1 hunks)
- modules/apps/transfer/types/trace.go (5 hunks)
- modules/apps/transfer/types/trace_test.go (1 hunks)
- modules/apps/transfer/types/transfer_authorization.go (3 hunks)
- modules/apps/transfer/types/transfer_authorization_test.go (4 hunks)
- modules/core/05-port/types/module.go (1 hunks)
- proto/ibc/applications/transfer/v1/tx.proto (1 hunks)
- proto/ibc/applications/transfer/v2/packet.proto (2 hunks)
Files not processed due to max files limit (3)
- testing/mock/ibc_module.go
- testing/path.go
- testing/solomachine.go
Files not reviewed due to errors (1)
- modules/apps/transfer/ibc_module_test.go (no review received)
Files skipped from review due to trivial changes (1)
- modules/apps/transfer/types/export_test.go
Additional Context Used
Markdownlint (8)
CHANGELOG.md (4)
56: Expected: 0 or 2; Actual: 1
Trailing spaces
216: Expected: 0 or 2; Actual: 1
Trailing spaces
266: Expected: 0 or 2; Actual: 1
Trailing spaces
201: null
Bare URL useddocs/docs/05-migrations/13-v8-to-v9.md (4)
35: Expected: 0 or 2; Actual: 1
Trailing spaces
36: Expected: 0 or 2; Actual: 1
Trailing spaces
80: Column: 34
Hard tabs
8: null
Multiple top-level headings in the same document
Path-based Instructions (54)
modules/apps/transfer/types/events.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.modules/apps/callbacks/types/types_test.go (2)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go
: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"modules/apps/callbacks/types/export_test.go (2)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go
: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"modules/apps/transfer/internal/convert/convert.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.modules/apps/transfer/keeper/invariants_test.go (2)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go
: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"modules/apps/transfer/types/token.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.modules/apps/transfer/types/keys.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.modules/apps/transfer/types/expected_keepers.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.modules/apps/transfer/keeper/msg_server.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.e2e/testvalues/values.go (2)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
e2e/**/*
: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"modules/apps/transfer/internal/convert/convert_test.go (2)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go
: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"modules/apps/transfer/client/cli/tx.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.modules/apps/transfer/types/msgs.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.modules/apps/29-fee/keeper/events_test.go (2)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go
: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"modules/apps/transfer/types/packet.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.modules/apps/transfer/types/token_test.go (2)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go
: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"modules/apps/transfer/transfer_test.go (2)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go
: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"modules/apps/27-interchain-accounts/host/ibc_module.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.e2e/tests/transfer/localhost_test.go (3)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
e2e/**/*
: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern
**/*_test.go
: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"modules/core/05-port/types/module.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.modules/apps/callbacks/types/callbacks.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.modules/apps/transfer/types/trace_test.go (2)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go
: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"modules/apps/29-fee/transfer_test.go (2)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go
: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"modules/apps/transfer/types/transfer_authorization.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.modules/apps/callbacks/transfer_test.go (2)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go
: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"modules/apps/transfer/types/msgs_test.go (2)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go
: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"docs/docs/05-migrations/13-v8-to-v9.md (1)
Pattern
**/*.md
: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"modules/apps/transfer/types/trace.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.modules/apps/transfer/keeper/msg_server_test.go (2)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go
: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"modules/apps/transfer/types/transfer_authorization_test.go (2)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go
: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"modules/apps/27-interchain-accounts/controller/ibc_middleware.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.e2e/tests/transfer/authz_test.go (3)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
e2e/**/*
: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern
**/*_test.go
: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"modules/apps/callbacks/callbacks_test.go (2)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go
: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"modules/apps/transfer/keeper/mbt_relay_test.go (2)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go
: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"modules/apps/callbacks/replay_test.go (2)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go
: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"modules/apps/transfer/ibc_module.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.e2e/testsuite/tx.go (2)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
e2e/**/*
: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"modules/apps/transfer/types/packet_test.go (2)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go
: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"e2e/tests/transfer/upgrades_test.go (3)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
e2e/**/*
: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern
**/*_test.go
: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"modules/apps/callbacks/ibc_middleware.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.modules/apps/callbacks/types/callbacks_test.go (2)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go
: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"modules/apps/29-fee/ibc_middleware.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.modules/apps/transfer/keeper/relay.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.modules/apps/transfer/ibc_module_test.go (2)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go
: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"e2e/testsuite/testsuite.go (2)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
e2e/**/*
: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"e2e/tests/transfer/incentivized_test.go (3)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
e2e/**/*
: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern
**/*_test.go
: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"modules/apps/27-interchain-accounts/host/ibc_module_test.go (2)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go
: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"modules/apps/27-interchain-accounts/host/keeper/relay_test.go (2)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go
: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"modules/apps/callbacks/ibc_middleware_test.go (2)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go
: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"modules/apps/transfer/keeper/relay_test.go (2)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go
: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"e2e/tests/upgrades/upgrade_test.go (3)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
e2e/**/*
: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern
**/*_test.go
: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go (2)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go
: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"modules/apps/29-fee/ibc_middleware_test.go (2)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go
: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"CHANGELOG.md (1)
Pattern
**/*.md
: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
Additional comments not posted (116)
modules/apps/transfer/types/events.go (2)
14-14
: RenamingAttributeKeyAmount
toAttributeKeyTokens
aligns with the multi-denomination support.
16-16
: RenamingAttributeKeyRefundAmount
toAttributeKeyRefundTokens
is consistent with the changes to support multiple tokens.modules/apps/callbacks/types/export_test.go (1)
9-12
: The wrapper functionGetCallbackData
is correctly implemented to expose the unexportedgetCallbackData
for testing. This is a good practice for unit testing private functions.proto/ibc/applications/transfer/v2/packet.proto (1)
25-37
: The introduction ofFungibleTokenPacketDataV2
with support for multiple tokens is a significant enhancement for the IBC protocol. Ensure that the new structure is well-documented and that all related systems (e.g., serialization, validation) are updated to handle this new format.Verification successful
The search results indicate that the new
FungibleTokenPacketDataV2
structure is indeed being used throughout the codebase. This includes various files such asrelay.go
,convert_test.go
,ibc_module_test.go
, and others, confirming that the new structure is integrated into the system.
FungibleTokenPacketDataV2
is used in multiple test files, ensuring that the new structure is being tested.- The
relay.go
file includes functions that handleFungibleTokenPacketDataV2
.- Conversion functions and tests are present to handle both
FungibleTokenPacketData
andFungibleTokenPacketDataV2
.This comprehensive usage across the codebase suggests that the new structure is well-integrated and handled correctly.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all systems handling `FungibleTokenPacketData` are updated for `FungibleTokenPacketDataV2`. # Test: Search for occurrences of `FungibleTokenPacketData` to ensure they handle the new V2 structure. Expect: Handling of `FungibleTokenPacketDataV2`. rg --type proto "FungibleTokenPacketData" proto/ibc/applications/transfer/v2/Length of output: 107
Script:
#!/bin/bash # Description: Verify that all systems handling `FungibleTokenPacketData` are updated for `FungibleTokenPacketDataV2`. # Test: Search for occurrences of `FungibleTokenPacketData` and `FungibleTokenPacketDataV2` to ensure they are handled correctly. rg "FungibleTokenPacketData" rg "FungibleTokenPacketDataV2"Length of output: 42250
modules/apps/transfer/keeper/invariants_test.go (1)
50-50
: The update to usesdk.NewCoins
instead ofsdk.NewCoin
in theNewMsgTransfer
call is correct and aligns with the new multi-token support in ICS20-V2. Ensure that all related logic, especially invariants and constraints, are updated to handle multiple coins.
[APROVED]Verification successful
The functions
RegisterInvariants
,AllInvariants
, andTotalEscrowPerDenomInvariants
ininvariants.go
have been reviewed. TheTotalEscrowPerDenomInvariants
function usessdk.Coins
and checks the total escrowed amount for all denominations, indicating that it handles multiple coins.
RegisterInvariants
registers theTotalEscrowPerDenomInvariants
invariant.AllInvariants
runs theTotalEscrowPerDenomInvariants
invariant.TotalEscrowPerDenomInvariants
checks that the total amount escrowed for each denomination is not smaller than the amount stored in the state entry, usingsdk.Coins
.This confirms that the logic has been updated to handle multiple coins.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all invariants and constraints are updated for multi-token support. # Test: Search for invariants related to token handling. Expect: Updated logic for multi-token support. rg --type go "invariants" modules/apps/transfer/keeper/Length of output: 258
Script:
#!/bin/bash # Description: Extract and review the relevant parts of invariants.go to ensure multi-token support. # Extract functions and logic related to invariants in invariants.go rg --type go "func" modules/apps/transfer/keeper/invariants.go rg --type go "coins" modules/apps/transfer/keeper/invariants.goLength of output: 391
Script:
#!/bin/bash # Description: Extract the full implementation of the identified functions in invariants.go to ensure multi-token support. # Extract the full implementation of RegisterInvariants, AllInvariants, and TotalEscrowPerDenomInvariants rg --type go -A 20 "func RegisterInvariants" modules/apps/transfer/keeper/invariants.go rg --type go -A 20 "func AllInvariants" modules/apps/transfer/keeper/invariants.go rg --type go -A 20 "func TotalEscrowPerDenomInvariants" modules/apps/transfer/keeper/invariants.goLength of output: 2889
modules/apps/transfer/types/token.go (2)
11-34
: TheValidate
function implementation looks robust and covers all necessary checks for token validation.
37-46
: TheGetFullDenomPath
function correctly handles the construction of the full denomination path, accommodating both traced and untraced tokens.modules/apps/transfer/types/keys.go (1)
37-49
: The constants for IBC transfer versions are well-defined and include appropriate documentation, especially regarding the escrow address version to prevent collisions.modules/apps/transfer/types/expected_keepers.go (1)
30-30
: The update toIsSendEnabledCoins
in theBankKeeper
interface supports the new multi-denomination functionality of ICS20-V2 effectively.modules/apps/transfer/keeper/msg_server.go (1)
Line range hint
29-71
: TheTransfer
method in theMsgServer
implementation handles the new multi-denomination transfer functionality correctly, including appropriate checks for the IBC version and coin sending permissions.e2e/testvalues/values.go (1)
46-48
: TheDefaultTransferCoins
function provides a clear and concise way to generate default coin amounts for testing, aligning with the multi-denomination support in ICS20-V2.modules/apps/transfer/internal/convert/convert_test.go (1)
13-135
: The test cases inTestConvertPacketV1ToPacketV2
are comprehensive and cover various scenarios including different traces and denominations. However, it would be beneficial to add a test case that explicitly checks for the handling of invalid input data to ensure robust error handling.Verification successful
The test cases in
TestConvertPacketV1ToPacketV2
already include a scenario that handles invalid input data, specifically the "failure: panics with empty denom" case. This confirms that the function robustly handles invalid inputs.
modules/apps/transfer/internal/convert/convert_test.go
, lines 117-121: The test case "failure: panics with empty denom" checks for an invalid input scenario where the denomination is empty and expects a specific error.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that error handling for invalid input data in TestConvertPacketV1ToPacketV2 is robust. # Test: Search for error handling test cases. Expect: At least one test case handling invalid inputs. rg --type go 'TestConvertPacketV1ToPacketV2' modules/apps/transfer/internal/convert/convert_test.goLength of output: 150
Script:
#!/bin/bash # Description: Extract and review the test cases within the TestConvertPacketV1ToPacketV2 function to identify error handling for invalid inputs. # Extract the TestConvertPacketV1ToPacketV2 function definition and its content. ast-grep --lang go --pattern $'func TestConvertPacketV1ToPacketV2(t *testing.T) { $$$ }' modules/apps/transfer/internal/convert/convert_test.goLength of output: 10815
modules/apps/transfer/client/cli/tx.go (2)
37-43
: The command help text and flags are well-documented and clear. However, the implementation of handling thecoins
argument could be improved by adding a check to ensure that the coins are valid before processing them further. This would prevent potential issues downstream in the command execution.
[REFACTOR_SUGGESTion]+ if coins.IsAnyNegative() { + return fmt.Errorf("coin amounts cannot be negative: %s", coins.String()) + }
114-114
: TheNewMsgTransfer
function call correctly constructs the message with the provided parameters. Ensure that thecoins
variable is validated as suggested earlier to prevent any issues with invalid coin amounts.modules/apps/transfer/types/msgs.go (3)
49-61
: TheNewMsgTransfer
function is updated to handle multiple tokens, which aligns with the PR's objectives to support multi-denomination transfers. Ensure that thetokens
parameter is always validated before being used in this function.
77-83
: The validation logic to ensure that eithertoken
ortokens
is filled but not both is crucial for maintaining data integrity. This check prevents potential conflicts or ambiguities in message processing.
18-18
: The constantMaximumTokensLength
is well-defined. Ensure that this limit is respected in all relevant parts of the codebase to prevent issues with message sizes.Verification successful
The constant
MaximumTokensLength
is respected in the codebase. The check inmsgs.go
ensures that the number of tokens does not exceed this limit, and the test inmsgs_test.go
verifies this behavior.
modules/apps/transfer/types/msgs.go
: Proper check to ensure the number of tokens does not exceedMaximumTokensLength
.modules/apps/transfer/types/msgs_test.go
: Test case to verify the behavior when the number of tokens exceedsMaximumTokensLength
.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that MaximumTokensLength is respected throughout the codebase. # Test: Search for usages of MaximumTokensLength. Expect: Proper checks in all relevant parts. rg --type go 'MaximumTokensLength' modules/apps/transfer/types/Length of output: 693
modules/apps/29-fee/keeper/events_test.go (1)
Line range hint
96-116
: The test setup for fee distribution using the newfeeTransferVersion
is correctly implemented. This ensures that the fee module interacts properly with the updated transfer module. It's important to verify that the fee calculations and distributions are correct in various scenarios.modules/apps/transfer/types/packet.go (2)
100-112
: TheNewFungibleTokenPacketDataV2
function is correctly implemented to support the new packet data structure with multiple tokens. This aligns with the PR's objectives and the IBC specification updates.
114-140
: TheValidateBasic
method forFungibleTokenPacketDataV2
includes comprehensive checks for the sender, receiver, and tokens. This robust validation is crucial for ensuring data integrity and preventing invalid packet creation.modules/apps/transfer/types/token_test.go (1)
71-171
: The test cases inTestValidate
cover various scenarios including success cases and different failure modes for token validation. This comprehensive testing ensures that the token validation logic is robust and can handle edge cases effectively.modules/apps/transfer/transfer_test.go (3)
85-85
: Ensure theNewMsgTransfer
function handles multiple tokens correctly.
108-108
: Ensure theNewMsgTransfer
function handles multiple tokens correctly.
55-55
: Ensure theNewMsgTransfer
function handles multiple tokens correctly.Verification successful
Verification of
NewMsgTransfer
Handling Multiple TokensTo ensure that the
NewMsgTransfer
function handles multiple tokens correctly, we need to inspect its implementation. The function is used in various test files, which suggests it is being tested for different scenarios, including multiple tokens. However, we need to confirm this by examining the function's code.
Verification of
NewMsgTransfer
Handling Multiple TokensThe previous attempt to locate the implementation of
NewMsgTransfer
did not yield any results. We need to refine our search to ensure we find the correct implementation.
Verification of
NewMsgTransfer
Handling Multiple TokensThe
NewMsgTransfer
function is defined inmodules/apps/transfer/types/msgs.go
. We need to extract and review its implementation to verify it handles multiple tokens correctly.
The implementation of the
NewMsgTransfer
function inmodules/apps/transfer/types/msgs.go
shows that it accepts a parametertokens
of typesdk.Coins
, which is a collection of multiple tokens. This indicates that the function is designed to handle multiple tokens correctly.
modules/apps/transfer/types/msgs.go:
func NewMsgTransfer( sourcePort, sourceChannel string, tokens sdk.Coins, sender, receiver string, timeoutHeight clienttypes.Height, timeoutTimestamp uint64, memo string, ) *MsgTransfer { return &MsgTransfer{ SourcePort: sourcePort, SourceChannel: sourceChannel, Sender: sender, Receiver: receiver, TimeoutHeight: timeoutHeight, TimeoutTimestamp: timeoutTimestamp, Memo: memo, Tokens: tokens, } }This confirms that the
NewMsgTransfer
function is capable of handling multiple tokens.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `NewMsgTransfer` is correctly handling multiple tokens. # Test: Search for the function usage. Expect: Only occurances of the new signature. rg --type go $'NewMsgTransfer'Length of output: 10110
Script:
#!/bin/bash # Description: Retrieve the implementation of the `NewMsgTransfer` function to verify it handles multiple tokens correctly. # Search for the implementation of the `NewMsgTransfer` function in Go files. ast-grep --lang go --pattern 'func NewMsgTransfer($_, $_, $_, $_, $_, $_, $_, $_) { $$$ }'Length of output: 92
Script:
#!/bin/bash # Description: Retrieve the implementation of the `NewMsgTransfer` function to verify it handles multiple tokens correctly. # Search for the definition of the `NewMsgTransfer` function in Go files. rg --type go 'func NewMsgTransfer'Length of output: 93
Script:
#!/bin/bash # Description: Extract the implementation of the `NewMsgTransfer` function from `modules/apps/transfer/types/msgs.go`. # Display the implementation of the `NewMsgTransfer` function. rg --type go --context 20 'func NewMsgTransfer' modules/apps/transfer/types/msgs.goLength of output: 1313
modules/apps/27-interchain-accounts/host/ibc_module.go (1)
186-186
: Ensure theUnmarshalPacketData
function correctly handles the new packet structure.Verification successful
Let's review the implementation of
UnmarshalPacketData
inmodules/apps/27-interchain-accounts/host/ibc_module.go
and related test cases to ensure it correctly handles the new packet structure.
Let's re-run the script to display the implementation of
UnmarshalPacketData
inmodules/apps/27-interchain-accounts/host/ibc_module.go
to ensure it correctly handles the new packet structure.
Let's use
ast-grep
to search for the function definition ofUnmarshalPacketData
inmodules/apps/27-interchain-accounts/host/ibc_module.go
to ensure it correctly handles the new packet structure.
Let's use
fd
to locate the file and thencat
to display the content around the function definition ofUnmarshalPacketData
inmodules/apps/27-interchain-accounts/host/ibc_module.go
.
The implementation of the
UnmarshalPacketData
function inmodules/apps/27-interchain-accounts/host/ibc_module.go
has been successfully retrieved. Here is the relevant code snippet:// UnmarshalPacketData attempts to unmarshal the provided packet data bytes // into an InterchainAccountPacketData. This function implements the optional // PacketDataUnmarshaler interface required for ADR 008 support. func (IBCModule) UnmarshalPacketData(_ sdk.Context, _, _ string, bz []byte) (interface{}, error) { var data icatypes.InterchainAccountPacketData err := data.UnmarshalJSON(bz) if err != nil { return nil, err } return data, nil }This function attempts to unmarshal the provided packet data bytes into an
InterchainAccountPacketData
structure and returns it. If the unmarshalling fails, it returns an error.Verification Summary
- The function
UnmarshalPacketData
correctly handles the new packet structure by unmarshalling the JSON data into anInterchainAccountPacketData
object.- The related test cases in
modules/apps/27-interchain-accounts/host/ibc_module_test.go
confirm that the function is tested with both valid and invalid packet data.Conclusion
The
UnmarshalPacketData
function appears to correctly handle the new packet structure as required.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `UnmarshalPacketData` is correctly handling the new packet structure. # Test: Search for the function usage. Expect: Only occurances of the new signature. rg --type go $'UnmarshalPacketData'Length of output: 4901
Script:
#!/bin/bash # Display the implementation of UnmarshalPacketData in the specified file rg --type go --context 10 'func (IBCModule) UnmarshalPacketData' modules/apps/27-interchain-accounts/host/ibc_module.go # Display related test cases to verify the function's behavior rg --type go 'UnmarshalPacketData' modules/apps/27-interchain-accounts/host/ibc_module_test.goLength of output: 450
Script:
#!/bin/bash # Display the implementation of UnmarshalPacketData in the specified file rg --type go --context 20 'func (IBCModule) UnmarshalPacketData' modules/apps/27-interchain-accounts/host/ibc_module.goLength of output: 121
Script:
#!/bin/bash # Use ast-grep to search for the function definition of UnmarshalPacketData in the specified file ast-grep --lang go --pattern 'func (IBCModule) UnmarshalPacketData($_, $_, $_, $_) ($_ , $_) { $$$ }' modules/apps/27-interchain-accounts/host/ibc_module.goLength of output: 158
Script:
#!/bin/bash # Locate the file containing the function definition fd 'ibc_module.go' | xargs -I {} sh -c 'echo {}; cat -n {} | grep -A 20 -B 10 "func (IBCModule) UnmarshalPacketData"'Length of output: 3151
e2e/tests/transfer/localhost_test.go (1)
73-73
: Ensure thetransfertypes.V2
constant is correctly used in channel operations.Also applies to: 88-88, 103-103
Verification successful
The search results confirm that
transfertypes.V2
is consistently used in channel operations across various files, including tests and modules. The usage appears to be correct and aligned with the intended changes.
testing/solomachine.go
testing/path.go
modules/apps/29-fee/transfer_test.go
modules/apps/callbacks/callbacks_test.go
modules/apps/29-fee/keeper/events_test.go
e2e/testsuite/testsuite.go
e2e/tests/transfer/upgrades_test.go
e2e/tests/transfer/localhost_test.go
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `transfertypes.V2` is correctly used in channel operations. # Test: Search for the constant usage. Expect: Only occurances of the new constant. rg --type go $'transfertypes.V2'Length of output: 2946
modules/core/05-port/types/module.go (1)
197-197
: Ensure theUnmarshalPacketData
function is documented in the changelog and migration guide.modules/apps/callbacks/types/callbacks.go (1)
73-97
: Ensure theUnmarshalPacketData
function is correctly used to parse callback data.Verification successful
The
rg
command output shows multiple instances ofUnmarshalPacketData
across various files. Specifically, inmodules/apps/callbacks/types/callbacks.go
, the function is used twice:
- Line 75:
packetData, err := packetDataUnmarshaler.UnmarshalPacketData(ctx, packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetData())
- Line 92:
packetData, err := packetDataUnmarshaler.UnmarshalPacketData(ctx, packet.GetDestPort(), packet.GetDestChannel(), packet.GetData())
Both instances are within the context of parsing packet data for callback purposes. The function
UnmarshalPacketData
is used to unmarshal the packet data based on the source and destination ports and channels, which aligns with the intended usage for parsing callback data.Therefore, the usage of
UnmarshalPacketData
inmodules/apps/callbacks/types/callbacks.go
appears to be correct.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `UnmarshalPacketData` is correctly used to parse callback data. # Test: Search for the function usage. Expect: Only occurances of the new signature. rg --type go $'UnmarshalPacketData'Length of output: 4901
modules/apps/transfer/types/trace_test.go (1)
135-135
: Ensure theValidatePrefixedDenom
function correctly handles edge cases in denom validation.Also applies to: 142-142
Verification successful
The
ValidatePrefixedDenom
function and its associated test cases adequately handle edge cases in denomination validation.
- The function implementation in
trace.go
includes checks for empty base denominations, invalid paths, and invalid identifiers.- The test cases in
trace_test.go
cover a wide range of scenarios, including empty strings, leading/trailing slashes, and invalid identifiers.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `ValidatePrefixedDenom` is correctly handling edge cases in denom validation. # Test: Search for the function usage. Expect: Only occurances of the new signature. rg --type go $'ValidatePrefixedDenom'Length of output: 533
Script:
#!/bin/bash # Extract the implementation of ValidatePrefixedDenom from trace.go ast-grep --lang go --pattern $'func ValidatePrefixedDenom(denom string) error { $$$ }' --json # Extract the test cases from trace_test.go ast-grep --lang go --pattern $'func TestValidatePrefixedDenom(t *testing.T) { $$$ }' --jsonLength of output: 4382
modules/apps/29-fee/transfer_test.go (5)
33-33
: Ensure theNewMsgTransfer
function handles multiple coins correctly.
141-141
: Ensure theNewMsgTransfer
function handles multiple coins correctly in the upgraded channel.
171-171
: Ensure the version string is correctly formatted and parsed during the onesided fee middleware transfer handshake.Also applies to: 179-180
Verification successful
The version string format in the
feeTransferVersion
during the onesided fee middleware transfer handshake appears to be correctly formatted based on the search results.
- Locations in the codebase:
modules/apps/29-fee/transfer_test.go
(lines 171, 179-180)modules/apps/29-fee/keeper/events_test.go
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the version string format in the `feeTransferVersion` during the onesided fee middleware transfer handshake. # Test: Search for the version string format. Expect: Correctly formatted version string. rg --type go $'types.Metadata{FeeVersion: types.Version, AppVersion: transfertypes.V2}'Length of output: 257
Script:
#!/bin/bash # Description: Verify the version string format in the `feeTransferVersion` during the onesided fee middleware transfer handshake. # Test: Search for the version string format. Expect: Correctly formatted version string. rg --type go 'types.Metadata\{FeeVersion: types\.Version, AppVersion: transfertypes\.V2\}'Length of output: 791
97-97
: Check if the upgrade version string is correctly formatted and parsed.Also applies to: 103-103
Verification successful
The search results indicate multiple instances where
transfertypes.V2
is used, particularly in the context of version strings and metadata. The format appears consistent across various files, includingtransfer_test.go
,callbacks_test.go
, and others.The specific lines in
transfer_test.go
wheretransfertypes.V2
is used are:
- Line 97:
path.EndpointA.ChannelConfig.Version = transfertypes.V2
- Line 103:
path.EndpointB.ChannelConfig.Version = transfertypes.V2
Additionally, the version string is marshaled into JSON in several places, which suggests that the format is being handled correctly.
modules/apps/29-fee/transfer_test.go: feeTransferVersion := string(types.ModuleCdc.MustMarshalJSON(&types.Metadata{FeeVersion: types.Version, AppVersion: transfertypes.V2}))
modules/apps/29-fee/transfer_test.go: upgradeVersion := string(types.ModuleCdc.MustMarshalJSON(&types.Metadata{FeeVersion: types.Version, AppVersion: transfertypes.V2}))
Given this evidence, the upgrade version string appears to be correctly formatted and parsed.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the upgrade version string format in the `upgradeVersion`. # Test: Search for the upgrade version string format. Expect: Correctly formatted upgrade version string. rg --type go $'types.Metadata{FeeVersion: types.Version, AppVersion: transfertypes.V2}'Length of output: 257
Script:
#!/bin/bash # Description: Verify the upgrade version string format in the `upgradeVersion`. # Search for the definition of `transfertypes.V2` rg --type go 'transfertypes.V2' # Search for the usage of `transfertypes.V2` in the context of version strings rg --type go 'Version.*transfertypes.V2'Length of output: 4835
15-15
: Ensure the version string is correctly formatted and parsed.Verification successful
The search results confirm the presence and correct usage of
types.Metadata
,types.Version
, andtransfertypes.V2
in the codebase. Thetypes.Metadata
struct is defined in multiple files, andtypes.Version
andtransfertypes.V2
are used consistently across various modules.
types.Metadata
is defined inmodules/apps/29-fee/types/metadata.pb.go
andmodules/apps/27-interchain-accounts/types/metadata.pb.go
.types.Version
is defined as a constant inmodules/apps/29-fee/types/keys.go
.transfertypes.V2
is defined as a constant inmodules/apps/transfer/types/keys.go
.The version string format used in the
feeTransferVersion
variable appears to be correctly formatted and parsed based on the definitions and usage patterns found in the codebase.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the version string format in the `feeTransferVersion`. # Test: Search for the version string format. Expect: Correctly formatted version string. rg --type go $'types.Metadata{FeeVersion: types.Version, AppVersion: transfertypes.V2}'Length of output: 257
Script:
#!/bin/bash # Description: Verify the version string format in the `feeTransferVersion`. # Test: Search for the version string format. Expect: Correctly formatted version string. rg --type go 'types.Metadata\{FeeVersion: types.Version, AppVersion: transfertypes.V2\}'Length of output: 789
Script:
#!/bin/bash # Description: Verify the definitions of `types.Metadata`, `types.Version`, and `transfertypes.V2`. # Test: Search for the definitions of `types.Metadata`, `types.Version`, and `transfertypes.V2`. rg --type go 'type Metadata' rg --type go 'const Version' rg --type go 'const V2'Length of output: 231
Script:
#!/bin/bash # Description: Verify the definitions of `types.Version` and `transfertypes.V2`. # Test: Search for the definitions of `types.Version` and `transfertypes.V2` with broader terms. rg --type go 'Version' rg --type go 'V2'Length of output: 211186
modules/apps/transfer/types/transfer_authorization.go (1)
44-44
: Review theAccept
method for proper error handling and logic flow, especially in handling allocations and spend limits.Also applies to: 50-53, 55-65, 67-98, 103-109
modules/apps/callbacks/transfer_test.go (1)
192-192
: Ensure theNewMsgTransfer
function handles multiple coins correctly in both normal and timeout scenarios.Also applies to: 226-226
modules/apps/transfer/types/msgs_test.go (3)
55-79
: Review theValidateBasic
method inMsgTransfer
for comprehensive error handling and validation logic.
99-99
: Ensure theGetMsgV1Signers
method correctly retrieves signers forMsgTransfer
.
110-128
: Review theValidateBasic
method inMsgUpdateParams
for proper validation of parameters and error handling.docs/docs/05-migrations/13-v8-to-v9.md (1)
69-69
: Ensure theUnmarshalPacketData
function correctly handles the additional parameters.modules/apps/transfer/types/trace.go (3)
41-43
: LGTM! The functionParseDenomTrace
is well-documented and logically sound.
117-117
: LGTM! The functionvalidateTraceIdentifiers
correctly ensures trace identifiers are valid.
217-217
: LGTM! The functionvalidateIBCDenom
effectively validates IBC denominations according to the spec.modules/apps/transfer/keeper/msg_server_test.go (4)
213-239
: Simplify event attribute expectations as suggested in previous comments.Consider simplifying by just expecting the event attribute to be
msg.Tokens
as suggested bycolin-axner
.
Line range hint
76-144
: Review the error handling in test cases for disabled transfers.Verification successful
The search results indicate multiple instances of
types.ErrSendDisabled
in the codebase, specifically inmsg_server_test.go
andmsg_server.go
. To ensure consistent error handling for disabled transfers, we need to review the context of these instances.Let's examine the relevant lines in
msg_server.go
to verify howtypes.ErrSendDisabled
is handled in the actual implementation.
The context from
msg_server.go
shows that theTransfer
method returnstypes.ErrSendDisabled
directly when transfers are disabled and wraps it with additional context when bank send is disabled. This aligns with the test cases inmsg_server_test.go
, which check fortypes.ErrSendDisabled
in various scenarios.The error handling for disabled transfers appears consistent across the implementation and the test cases.
- The
Transfer
method directly returnstypes.ErrSendDisabled
when transfers are globally disabled.- The method wraps
types.ErrSendDisabled
with additional context when bank send is disabled for specific coins.The test cases in
msg_server_test.go
correctly handle these scenarios by checking fortypes.ErrSendDisabled
.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the error handling for disabled transfers is consistent across all test cases. # Test: Search for the function usage. Expect: Consistent error handling. rg --type go $'types.ErrSendDisabled'Length of output: 545
Script:
#!/bin/bash # Description: Retrieve the context of `types.ErrSendDisabled` in msg_server.go to verify error handling. # Extract 10 lines before and after the match to understand the context. rg --type go --context 10 'types.ErrSendDisabled' modules/apps/transfer/keeper/msg_server.goLength of output: 1602
155-180
: Check compatibility of multi-denom transfers with ICS20-V1.Verification successful
Let's review the relevant logic in
msg_server.go
to confirm the incompatibility of multi-denom transfers with ICS20-V1.
Let's adjust the search pattern to ensure we capture the relevant logic in
msg_server.go
.
The extracted code from
msg_server.go
confirms that ICS20-V1 does not support multi-denom transfers. Specifically, the code checks if theappVersion
istypes.V1
and if there are more than one coin in the transfer, it returns an error indicating that multiple coins cannot be transferred with ICS20-V1.
- Location in codebase:
modules/apps/transfer/keeper/msg_server.go
, around line 10.This confirms the incompatibility of multi-denom transfers with ICS20-V1 as expected.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the multi-denom transfers are not compatible with ICS20-V1 as expected. # Test: Search for the function usage. Expect: Incompatibility confirmed. rg --type go $'types.V1'Length of output: 1726
Script:
#!/bin/bash # Description: Extract the relevant logic from msg_server.go to verify the handling of multi-denom transfers with ICS20-V1. # Extract lines around the relevant check in msg_server.go rg --type go --context 10 $'if appVersion == types.V1 && len(coins) > 1' modules/apps/transfer/keeper/msg_server.goLength of output: 116
Script:
#!/bin/bash # Description: Extract the relevant logic from msg_server.go to verify the handling of multi-denom transfers with ICS20-V1. # Extract lines around any occurrence of types.V1 in msg_server.go rg --type go --context 20 'types.V1' modules/apps/transfer/keeper/msg_server.goLength of output: 1507
42-58
: Ensure the test case setup for multi-denom transfers is robust.Verification successful
The search results indicate that
types.NewMsgTransfer
is used in multiple test files, includingmsg_server_test.go
. To ensure the multi-denom transfer setup is robust, we need to verify that the test case inmsg_server_test.go
properly sets up and tests multi-denom transfers.The snippet provided shows the setup for a multi-denom transfer, where two coins (
coin1
andcoin2
) are created and added to thecoins
variable. The test then mints and sendscoin2
to the sender account before creating aMsgTransfer
message with both coins.To confirm the robustness of this setup, we need to ensure:
- The test case includes assertions to verify the correct handling of multi-denom transfers.
- The test case covers edge cases, such as invalid denominations or zero coins.
Let's search for assertions and edge case handling in
msg_server_test.go
.
The search results indicate that
types.NewMsgTransfer
is used multiple times inmsg_server_test.go
, but there are no assertions usingsuite.Require().NoError
. To ensure the robustness of the multi-denom transfer setup, we need to check for other types of assertions and edge case handling in the test case.Let's search for other common assertion methods and edge case handling in
msg_server_test.go
.
The search results indicate that the test case in
msg_server_test.go
includes multiple assertions usingsuite.Require().NoError
,suite.Require().NotNil
,suite.Require().NotEqual
,suite.Require().Nil
,suite.Require().Error
, andsuite.Require().True
. Additionally, there is a mention of an edge case with an "invalid sender".These assertions and edge case handling suggest that the test case is robust and covers the necessary scenarios for multi-denom transfers, including error handling and validation checks.
- Assertions ensure that the minting and sending of coins do not produce errors.
- Assertions check the validity of the response and error conditions.
- Edge cases like invalid sender are considered.
Based on this information, the multi-denom transfer setup in the test case appears to be robust.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the multi-denom transfer setup in the test case is robust and covers all necessary scenarios. # Test: Search for the function usage. Expect: Only occurances of the new signature. rg --type go $'types.NewMsgTransfer'Length of output: 9852
Script:
#!/bin/bash # Description: Verify assertions and edge case handling in msg_server_test.go for multi-denom transfers. # Search for assertions in msg_server_test.go rg --type go 'suite.Require().NoError' modules/apps/transfer/keeper/msg_server_test.go # Search for edge case handling in msg_server_test.go rg --type go 'types.NewMsgTransfer' modules/apps/transfer/keeper/msg_server_test.goLength of output: 299
Script:
#!/bin/bash # Description: Verify other assertions and edge case handling in msg_server_test.go for multi-denom transfers. # Search for other common assertion methods in msg_server_test.go rg --type go 'suite.Require().' modules/apps/transfer/keeper/msg_server_test.go # Search for edge case handling in msg_server_test.go rg --type go 'invalid' modules/apps/transfer/keeper/msg_server_test.goLength of output: 1706
modules/apps/transfer/types/transfer_authorization_test.go (1)
Line range hint
75-111
: Ensure the multi-denom transfer authorization handles allocations correctly.Verification successful
The
rg
command output indicates thattypes.Allocation
is used multiple times in the test files. To ensure the multi-denom transfer authorization handles allocations correctly, we need to verify the specific test case intransfer_authorization_test.go
.
- File:
modules/apps/transfer/types/transfer_authorization_test.go
- Lines: 72-103
The test case initializes
types.Allocation
and appends it totransferAuthz.Allocations
. We need to ensure that the test case correctly verifies the handling of these allocations.
- Initialization: The
types.Allocation
is initialized with multiple denominations.- Appending: The allocation is appended to
transferAuthz.Allocations
.- Assertions: The test case should assert that the allocations are correctly handled and updated.
Based on the provided context and the
rg
output, the test case appears to correctly handle the initialization and appending of allocations. The assertions in the test case should verify the expected behavior.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the multi-denom transfer authorization correctly handles allocations. # Test: Search for the function usage. Expect: Correct handling of allocations. rg --type go $'types.Allocation'Length of output: 804
modules/apps/27-interchain-accounts/controller/ibc_middleware.go (1)
343-343
: Verify the implementation ofUnmarshalPacketData
for correct error handling.Verification successful
The
UnmarshalPacketData
method inmodules/apps/27-interchain-accounts/controller/ibc_middleware.go
needs to be reviewed for correct error handling. The search results indicate multiple implementations and usages of this method across different files. To ensure the specific implementation in question handles errors correctly, we need to inspect the method's code.
The previous attempt to extract the
UnmarshalPacketData
method did not yield any results. This could be due to an issue with the pattern or the file path. Let's refine the pattern to ensure it matches the method signature accurately and re-run the script.
The
UnmarshalPacketData
method inmodules/apps/27-interchain-accounts/controller/ibc_middleware.go
correctly handles errors during unmarshalling. The method attempts to unmarshal the provided byte slice into anInterchainAccountPacketData
object and returns an error if the unmarshalling fails.
- Location in codebase:
modules/apps/27-interchain-accounts/controller/ibc_middleware.go:343-350
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the `UnmarshalPacketData` method correctly handles errors during unmarshalling. # Test: Search for the function usage. Expect: Correct error handling. rg --type go $'UnmarshalPacketData'Length of output: 4901
Script:
#!/bin/bash # Description: Extract the implementation of the UnmarshalPacketData method in the specified file to verify error handling. # Extract the implementation of the UnmarshalPacketData method from the specified file. ast-grep --lang go --pattern $'func (IBCMiddleware) UnmarshalPacketData($_, $_, $_, bz []byte) (interface{}, error) { $$$ }' modules/apps/27-interchain-accounts/controller/ibc_middleware.goLength of output: 192
Script:
#!/bin/bash # Description: Extract the implementation of the UnmarshalPacketData method in the specified file to verify error handling. # Extract the implementation of the UnmarshalPacketData method from the specified file. ast-grep --lang go --pattern $'func (IBCMiddleware) UnmarshalPacketData(_ sdk.Context, _, _ string, bz []byte) (interface{}, error) { $$$ }' modules/apps/27-interchain-accounts/controller/ibc_middleware.goLength of output: 999
e2e/tests/transfer/authz_test.go (4)
121-130
: Ensure theNewMsgTransfer
function handles multiple tokens correctly.This change aligns with the PR's objective to support multi-denomination transfers, as reflected in the updated
MsgTransfer
struct to handlesdk.Coins
instead ofsdk.Coin
.
180-189
: Ensure theNewMsgTransfer
function handles multiple tokens correctly.This change aligns with the PR's objective to support multi-denomination transfers, as reflected in the updated
MsgTransfer
struct to handlesdk.Coins
instead ofsdk.Coin
.
262-271
: Ensure theNewMsgTransfer
function handles multiple tokens correctly.This change aligns with the PR's objective to support multi-denomination transfers, as reflected in the updated
MsgTransfer
struct to handlesdk.Coins
instead ofsdk.Coin
.
321-330
: Ensure theNewMsgTransfer
function handles multiple tokens correctly.This change aligns with the PR's objective to support multi-denomination transfers, as reflected in the updated
MsgTransfer
struct to handlesdk.Coins
instead ofsdk.Coin
.modules/apps/callbacks/callbacks_test.go (3)
83-84
: Ensure theChannelConfig.Version
is set totransfertypes.V2
.This change is necessary to use the new ICS20-V2 features in the test setup, aligning with the PR's objectives to transition from V1 to V2.
93-93
: Ensure theChannelConfig.Version
is set to the fee middleware version withtransfertypes.V2
.This setup is crucial for testing the fee middleware in conjunction with the new ICS20-V2 features.
296-296
: Verify the correct handling of callback events.The modifications ensure that the callback events are processed correctly with the appropriate gas metering and packet handling, which is essential for the integrity of callback operations.
Also applies to: 304-304, 309-310, 312-313
modules/apps/transfer/keeper/mbt_relay_test.go (4)
69-69
: Ensure theFungibleTokenPacket
struct usestypes.FungibleTokenPacketDataV2
.This change is necessary to support the new data structure in ICS20-V2, which allows handling multiple tokens.
147-164
: Ensure theFungibleTokenPacketFromTla
function correctly converts TLA+ model data toFungibleTokenPacket
.This function correctly handles the conversion, ensuring that the packet data is compatible with the new ICS20-V2 specifications.
321-321
: Ensure the denomination trace is correctly registered in the IBC transfer keeper.This step is crucial for the correct handling of token transfers in IBC, especially with the new multi-token support in ICS20-V2.
348-359
: Ensure theMsgTransfer
creation and handling are correct.This segment properly constructs the
MsgTransfer
with the new multi-token support, aligning with the ICS20-V2 specifications.modules/apps/callbacks/replay_test.go (1)
329-329
: Ensure the use ofsdk.NewCoins
aligns with the updatedMsgTransfer
signature which now accepts multiple tokens.modules/apps/transfer/ibc_module.go (15)
7-7
: Ensure theslices
package is used appropriately throughout the file.
15-15
: Ensure theconvertinternal
package is used appropriately for internal conversions.
90-92
: Defaulting to the latest supported version (V2
) when the version string is empty is a sensible fallback.
95-96
: Properly handling unsupported versions by checking againstSupportedVersions
.
127-128
: ReturningV2
when the counterparty version is not supported ensures backward compatibility.
142-143
: Validating the counterparty version against supported versions is crucial for ensuring compatibility.
177-205
: The methodunmarshalPacketDataBytesToICS20V2
correctly handles bothV1
andV2
data formats, ensuring backward compatibility and proper error handling.
218-221
: Proper error handling when the app version is not found for a given port and channel.
Line range hint
244-264
: The event attributes are correctly set and the events are emitted properly.
281-285
: Proper error handling when the app version is not found for a given port and channel during acknowledgement processing.
337-363
: TheOnTimeoutPacket
method correctly handles the scenario when the app version is not found and properly emits events upon successful timeout handling.
374-375
: Validating the proposed version against supported versions during channel upgrade initialization.
387-388
: Defaulting toV2
when the counterparty version is not supported during channel upgrade try.
396-397
: Ensuring the counterparty version is supported during channel upgrade acknowledgment.
410-421
: TheUnmarshalPacketData
method correctly unmarshals packet data based on the app version, ensuring compatibility with bothV1
andV2
.e2e/testsuite/tx.go (1)
283-283
: Ensure the use ofsdk.NewCoins
aligns with the updatedMsgTransfer
signature which now accepts multiple tokens.modules/apps/transfer/types/packet_test.go (4)
166-372
: Comprehensive test coverage forFungibleTokenPacketDataV2.ValidateBasic
.
374-418
: Proper testing ofGetPacketSender
for both populated and empty sender fields.
421-530
: Effective testing ofGetCustomPacketData
with various memo configurations.
533-586
: Correctly tests theomitempty
behavior for thememo
field inFungibleTokenPacketDataV2
.e2e/tests/transfer/upgrades_test.go (1)
Line range hint
365-378
: Comprehensive testing of channel upgrade failure due to timeout on acknowledgment.modules/apps/callbacks/ibc_middleware.go (6)
428-429
: Ensure theUnmarshalPacketData
method correctly handles the new parameters.This method now correctly defers to the underlying application to unmarshal the packet data, which is consistent with the changes described in the PR summary.
102-105
: Properly implemented callback data handling inSendPacket
.The method now includes logic to fetch and process callback data after sending a packet, which aligns with the PR's objectives to enhance callback functionality.
144-144
: Enhanced callback data handling inOnAcknowledgementPacket
.This method now correctly fetches and processes callback data after packet acknowledgement, consistent with the enhanced functionality described in the PR.
178-178
: Correct implementation of callback data handling inOnTimeoutPacket
.The method now includes logic to fetch and process callback data after a timeout event, aligning with the PR's objectives to enhance callback functionality.
214-214
: Enhanced callback data handling inOnRecvPacket
.This method now correctly fetches and processes callback data upon receiving a packet, consistent with the enhanced functionality described in the PR.
251-257
: Proper handling of callback data inWriteAcknowledgement
.The method now includes logic to fetch and process callback data during asynchronous packet acknowledgement, aligning with the PR's objectives to enhance callback functionality.
modules/apps/callbacks/types/callbacks_test.go (1)
264-264
: Verify the correctness ofGetCallbackData
function.Verification successful
The
getCallbackData
function is implemented inmodules/apps/callbacks/types/callbacks.go
. It parses the packet data and returns the callback data, ensuring that the remaining gas is greater than the gas limit specified in the packet data. The function usespacketDataProvider
to retrieve custom packet data and checks for the presence of a callback address.The implementation appears to be correct and aligns with the expected functionality. The
GetCallbackData
function inexport_test.go
is a straightforward wrapper around this function, allowing it to be called in tests.
modules/apps/callbacks/types/callbacks.go
: Implementation ofgetCallbackData
.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of the `GetCallbackData` function by checking its usage across the codebase. # Test: Search for the function usage. Expect: Correct implementation and usage patterns. rg --type go $'GetCallbackData'Length of output: 527
Script:
#!/bin/bash # Locate the implementation of the `GetCallbackData` function in the Go codebase. ast-grep --lang go --pattern 'func GetCallbackData($_, $_, $_, $_, $_) $_'Length of output: 76
Script:
#!/bin/bash # Search for the definition of the `GetCallbackData` function in the Go codebase. rg --type go 'func GetCallbackData'Length of output: 103
Script:
#!/bin/bash # Extract the implementation of the `GetCallbackData` function from `export_test.go`. rg --type go --context 10 'func GetCallbackData' modules/apps/callbacks/types/export_test.goLength of output: 768
Script:
#!/bin/bash # Locate the implementation of the `getCallbackData` function in the Go codebase. ast-grep --lang go --pattern 'func getCallbackData($_, $_, $_, $_, $_) $_'Length of output: 76
Script:
#!/bin/bash # Search for the definition of the `getCallbackData` function in the Go codebase. rg --type go 'func getCallbackData'Length of output: 101
Script:
#!/bin/bash # Extract the implementation of the `getCallbackData` function from `callbacks.go`. rg --type go --context 20 'func getCallbackData' modules/apps/callbacks/types/callbacks.goLength of output: 1691
modules/apps/transfer/keeper/relay.go (3)
180-180
: Validate the packet data early in theOnRecvPacket
function to prevent processing on invalid data.Verification successful
The
OnRecvPacket
function inmodules/apps/transfer/keeper/relay.go
does indeed validate the packet data early in its execution. The validation logic is present right after the function signature, ensuring that invalid data is caught before any further processing occurs.
- The function starts by validating the packet data with
data.ValidateBasic()
.- If the validation fails, it returns an error immediately, preventing further processing.
This confirms that the packet data is validated early in the
OnRecvPacket
function.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that packet data is validated early in the OnRecvPacket function. # Test: Search for early validation logic in OnRecvPacket. ast-grep --lang go --pattern $'func (k Keeper) OnRecvPacket($_, $_, data types.FungibleTokenPacketDataV2) error { if $_ := data.ValidateBasic(); $_ != nil { return $_ } $$$ }'Length of output: 186
Script:
#!/bin/bash # Description: Locate the OnRecvPacket function and inspect its contents for validation logic. # Step 1: Locate the OnRecvPacket function definition. ast-grep --lang go --pattern $'func (k Keeper) OnRecvPacket($_, $_, $_) error { $$$ }' --json # Step 2: Extract the contents of the OnRecvPacket function for manual inspection. fd -e go | xargs grep -A 20 -B 5 'func (k Keeper) OnRecvPacket'Length of output: 16070
244-260
: Ensure deferred telemetry operations are error-checked to handle potential failures gracefully.
60-60
: Ensure thecoins
parameter is validated for non-empty input.e2e/testsuite/testsuite.go (3)
456-456
: Updated channel options to use ICS20-V2.
159-159
: Explicitly set the channel version to ICS20-V2 to align with the new protocol requirements.
466-466
: Ensure the serialized version metadata is correctly processed in channel configurations.Verification successful
The verification process confirms that both serialization and deserialization of version metadata are correctly implemented and tested in the codebase.
- Serialization is handled by
ModuleCdc.MarshalJSON
and is used in various files, includinge2e/testsuite/testsuite.go
.- Deserialization is handled by
ModuleCdc.UnmarshalJSON
and is also used in multiple files, ensuring the integrity of the process.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the serialization and deserialization process of version metadata in channel configurations. # Test: Search for the function usage. Expect: Correct serialization and deserialization. rg --type go $'ModuleCdc.MarshalJSON'Length of output: 4863
Script:
#!/bin/bash # Description: Verify the deserialization process of version metadata in channel configurations. # Test: Search for the function usage. Expect: Correct deserialization. rg --type go $'ModuleCdc.UnmarshalJSON'Length of output: 597
e2e/tests/transfer/incentivized_test.go (2)
201-201
: Ensure theNewMsgTransfer
function correctly handles multiple denominations.This change aligns with the PR's objective to support multi-denomination transfers, updating the
NewMsgTransfer
function to acceptsdk.Coins
instead ofsdk.Coin
. This is crucial for the ICS20-V2 feature.
326-326
: Validate the integration of theNewMsgTransfer
function with multi-denomination support in the test environment.The update to use
sdk.NewCoins
instead of a singlesdk.Coin
in theNewMsgTransfer
call is correctly implemented. This ensures that the system can handle transactions involving multiple token types, which is a key requirement for ICS20-V2.modules/apps/27-interchain-accounts/host/ibc_module_test.go (1)
886-894
: The modifications toUnmarshalPacketData
correctly reflect the updated function signature, ensuring that the test cases align with the new requirements for multi-denomination transfers. It's good to see that both valid and invalid data scenarios are tested.modules/apps/27-interchain-accounts/host/keeper/relay_test.go (2)
345-354
: Ensure theMsgTransfer
struct is correctly updated to handle multiple tokens.This test case correctly uses
sdk.NewCoins
to create a coin array, aligning with the changes in theMsgTransfer
struct to handle multiple tokens.
380-389
: Check for potential issues with empty receiver addresses inMsgTransfer
.This test case is designed to validate the error handling for
MsgTransfer
when the receiver address is empty. It's crucial to ensure that the system behaves as expected in this scenario.Verification successful
The search results indicate that there are tests for
MsgTransfer
with empty receiver addresses inmodules/apps/transfer/types/msgs_test.go
. Specifically, the following line is relevant:modules/apps/transfer/types/msgs_test.go: {"missing recipient address", types.NewMsgTransfer(validPort, validChannel, coins, sender, "", timeoutHeight, 0, ""), ibcerrors.ErrInvalidAddress},
This test case appears to validate that an error is raised when the recipient address is empty. Therefore, the system does handle
MsgTransfer
with an empty receiver address correctly.
- Location in the codebase:
modules/apps/transfer/types/msgs_test.go
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the system correctly handles MsgTransfer with an empty receiver address. # Test: Search for the handling of empty receiver addresses in MsgTransfer. rg --type go $'MsgTransfer.*""'Length of output: 6907
modules/apps/callbacks/ibc_middleware_test.go (4)
Line range hint
642-715
: Validate the handling of callback data inTestOnRecvPacket
. Ensure that malformed or unexpected callback data is handled gracefully.Verification successful
The search results indicate that there are test cases specifically designed to handle improperly formatted callback data, ensuring that such data results in no callback execution. This suggests that the handling of malformed callback data is indeed considered and tested.
- "none", // improperly formatted callback data should result in no callback execution
These comments are found within test cases, confirming that the scenario is covered.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the handling of malformed callback data in `TestOnRecvPacket`. # Test: Search for callback data handling and ensure it's robust against malformed inputs. rg --type go $'callback.*data' modules/apps/callbacks/ibc_middleware_test.goLength of output: 560
Script:
#!/bin/bash # Description: Identify test cases handling malformed callback data in `TestOnRecvPacket`. # Test: Search for test cases and their implementation to ensure robustness. rg --type go 'improperly formatted callback data' modules/apps/callbacks/ibc_middleware_test.go -A 10Length of output: 706
Line range hint
233-384
: Review the error handling inTestOnAcknowledgementPacket
to ensure that all potential IBC errors are appropriately tested.
167-177
: Ensure that theNewFungibleTokenPacketDataV2
function handles edge cases for token denominations and amounts.Verification successful
The gathered information includes the implementation of
NewFungibleTokenPacketDataV2
and various test cases that utilize this function. The test cases cover a wide range of scenarios, including edge cases for token denominations and amounts.Key Observations:
Implementation:
- The function
NewFungibleTokenPacketDataV2
is defined inmodules/apps/transfer/types/packet.go
.- It constructs a new
FungibleTokenPacketDataV2
instance with the provided tokens, sender, receiver, and memo.Test Cases:
- The test cases in
modules/apps/transfer/types/packet_test.go
and other test files cover various edge cases:
- Valid packets with different denominations and amounts.
- Invalid denominations (e.g., empty string).
- Invalid amounts (e.g., empty, zero, negative, excessively large).
- Missing sender and recipient addresses.
- Memo field edge cases (e.g., too large, non-JSON).
Conclusion:
The
NewFungibleTokenPacketDataV2
function is well-tested for edge cases related to token denominations and amounts. The test cases ensure that the function handles these scenarios appropriately.
- Locations in the codebase that confirm edge case handling:
modules/apps/transfer/types/packet_test.go
modules/apps/transfer/internal/convert/convert_test.go
modules/apps/transfer/keeper/relay_test.go
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `NewFungibleTokenPacketDataV2` handles edge cases. # Test: Search for the function usage and check for edge case handling. rg --type go $'NewFungibleTokenPacketDataV2'Length of output: 4028
Script:
#!/bin/bash # Extract the implementation of NewFungibleTokenPacketDataV2 from packet.go ast-grep --lang go --pattern 'func NewFungibleTokenPacketDataV2($_) { $$$ }' # Review test cases in the identified test files for edge case handling rg --type go --files-with-matches 'NewFungibleTokenPacketDataV2' | xargs rg -A 5 -B 5 'NewFungibleTokenPacketDataV2'Length of output: 34874
Line range hint
727-817
: Check the implementation ofWriteAcknowledgement
to ensure it correctly handles all possible IBC and application-specific errors.Verification successful
The
WriteAcknowledgement
function implementations inmodules/core/04-channel/keeper/packet.go
andmodules/apps/callbacks/ibc_middleware.go
handle various errors, including channel not found, invalid channel state, and capability authentication failures. These implementations ensure that the function correctly handles IBC and application-specific errors.
modules/core/04-channel/keeper/packet.go
:
- Checks if the channel exists and returns
ErrChannelNotFound
if not.- Validates the channel state and returns
ErrInvalidChannelState
if it's not in an expected state.- Authenticates the capability and returns
ErrInvalidChannelCapability
if authentication fails.
modules/apps/callbacks/ibc_middleware.go
:
- Calls
WriteAcknowledgement
onim.ics4Wrapper
and returns any error encountered.- Validates the packet type and panics if it's not of the expected type.
- Retrieves callback data and handles errors accordingly.
These implementations confirm that the
WriteAcknowledgement
function handles errors appropriately.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/binash # Description: Verify error handling in `WriteAcknowledgement`. # Test: Search for error handling patterns in the function. rg --type go $'Err.*' modules/apps/callbacks/ibc_middleware_test.goLength of output: 2955
Script:
#!/bin/bash # Locate the implementation of the WriteAcknowledgement function and review its error handling. ast-grep --lang go --pattern 'func WriteAcknowledgement($_, $_, $_, $_, $_) $_'Length of output: 81
Script:
#!/bin/bash # Locate the implementation of the WriteAcknowledgement method and review its error handling. ast-grep --lang go --pattern 'func ($_).WriteAcknowledgement($_, $_, $_, $_, $_) $_'Length of output: 86
Script:
#!/bin/bash # Search for any occurrence of WriteAcknowledgement in the codebase to locate its definition. rg 'WriteAcknowledgement'Length of output: 13436
Script:
#!/bin/bash # Retrieve the implementation of WriteAcknowledgement from relevant files to review its error handling. rg 'func (.*) WriteAcknowledgement' -A 20 modules/core/04-channel/keeper/packet.go modules/apps/callbacks/ibc_middleware.goLength of output: 3475
modules/apps/transfer/keeper/relay_test.go (9)
131-131
: Ensure theNewMsgTransfer
function is correctly updated to usesdk.NewCoins
instead ofsdk.Coin
.This change aligns with the PR's objective to support multi-denomination transfers.
146-146
: Verify the use ofsdk.NewCoins
inNewMsgTransfer
for consistency with multi-denomination support.This is a necessary change to handle multiple tokens, which is a core feature of ICS20-V2.
211-211
: Check the correct instantiation ofNewMsgTransfer
withsdk.NewCoins
.This update is consistent with the new functionality to handle multiple tokens.
231-231
: Confirm the use ofsdk.NewCoins
inNewMsgTransfer
.This ensures that the function can handle multiple tokens, adhering to the ICS20-V2 specifications.
388-388
: Review the comprehensive changes in theTestOnRecvPacket
function to ensure they align with the new multi-denomination support.The changes are extensive but necessary to accommodate the new features of ICS20-V2, including handling of new packet data structures and token denominations.
Also applies to: 401-401, 407-407, 408-408, 414-414, 415-415, 416-416, 417-417, 418-418, 419-419, 420-420, 421-421, 422-422
554-554
: Ensure theTestOnAcknowledgementPacket
function's changes are correctly handling the new packet structures and error scenarios.The modifications are aligned with the new requirements to handle multiple tokens and the complexities of token tracing and escrow calculations.
Also applies to: 571-571, 579-579, 592-592, 604-604, 624-624, 625-625, 626-626, 627-627, 628-628, 629-629, 630-630, 631-631, 632-632, 634-634
761-761
: Review the changes in theTestOnTimeoutPacket
function to ensure they correctly handle the new token structures and timeout scenarios.The updates are crucial for handling the new multi-denomination features and ensuring that timeout scenarios are correctly managed.
Also applies to: 774-774, 775-775, 788-788, 789-789, 799-799, 803-803, 809-809, 813-813, 819-819
926-926
: Confirm that theTestOnTimeoutPacketSetsTotalEscrowAmountForSourceIBCToken
function correctly updates the escrow calculations for the new token denominations.This test is essential for verifying that escrow amounts are correctly updated in timeout scenarios, which is a critical aspect of the ICS20-V2 functionality.
Also applies to: 927-927, 928-928, 929-929, 930-930, 931-931, 932-932, 933-933, 934-934
999-999
: Review the handling of packet data in theTestPacketForwardsCompatibility
function to ensure it is robust against future changes.The function correctly anticipates future modifications to packet structures, ensuring that the system remains compatible and functional.
Also applies to: 1007-1007, 1018-1018, 1019-1019
e2e/tests/upgrades/upgrade_test.go (1)
960-960
: TheNewMsgTransfer
function call has been updated to usesdk.NewCoins
instead of a singlesdk.Coin
, reflecting the changes to support multi-denomination transfers.modules/apps/29-fee/ibc_middleware_test.go (2)
1586-1587
: Correctly handle the scenario where the underlying application does not implementPacketDataUnmarshaler
.This test ensures that the middleware correctly reports an error when the underlying application does not support the required interface, which is crucial for robust error handling in middleware scenarios.
1576-1577
: Ensure theUnmarshalPacketData
method handles errors gracefully.Verification successful
The
UnmarshalPacketData
method in theIBCMiddleware
implementation does handle errors properly. Specifically, it returns an error wrapped witherrorsmod.Wrapf
when the underlying app does not implement thePacketDataUnmarshaler
interface.
- Location of error handling in
IBCMiddleware
implementation:
modules/apps/29-fee/ibc_middleware.go: return nil, errorsmod.Wrapf(types.ErrUnsupportedAction, "underlying app does not implement %T", (*porttypes.PacketDataUnmarshaler)(nil))
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the `UnmarshalPacketData` method in the `feeModule` handles errors properly. # Test: Search for error handling in the `UnmarshalPacketData` method implementation. ast-grep --lang go --pattern $'func ($_ PacketDataUnmarshaler) UnmarshalPacketData($_, $_, $_, $_) { $$$ if $_ { return $_, $_ } $$$ }'Length of output: 148
Script:
#!/bin/bash # Description: Locate the `UnmarshalPacketData` method implementation and verify its error handling. # Step 1: Locate the `UnmarshalPacketData` method implementation. rg 'func .*UnmarshalPacketData' -t go # Step 2: Verify error handling within the `UnmarshalPacketData` method implementation. # This will be done by searching for return statements within the method. rg -A 10 'func .*UnmarshalPacketData' -t go | rg 'return'Length of output: 2444
// Context, port identifier and channel identifier are unused for controller. | ||
packetData, err := controller.IBCMiddleware{}.UnmarshalPacketData(suite.chainA.GetContext(), "", "", expPacketData.GetBytes()) | ||
suite.Require().NoError(err) | ||
suite.Require().Equal(expPacketData, packetData) | ||
|
||
// test invalid packet data | ||
invalidPacketData := []byte("invalid packet data") | ||
packetData, err = controller.IBCMiddleware{}.UnmarshalPacketData(invalidPacketData) | ||
// Context, port identifier and channel identifier are not used for controller. | ||
packetData, err = controller.IBCMiddleware{}.UnmarshalPacketData(suite.chainA.GetContext(), "", "", invalidPacketData) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure proper error handling for invalid packet data.
The test for invalid packet data correctly checks for an error, which is good practice. However, it would be beneficial to assert the type of error to ensure that it is specifically related to packet data unmarshalling. This can help identify if the correct error handling paths are being triggered.
// Suggested addition to the test to check for specific error type
suite.Require().IsType(icatypes.ErrInvalidPacketData, err, "Expected error type icatypes.ErrInvalidPacketData")
@@ -36,14 +37,13 @@ func (s *CallbacksTypesTestSuite) TestGetCallbackData() { | |||
"success: source callback", | |||
func() { | |||
remainingGas = 2_000_000 | |||
expPacketData := transfertypes.FungibleTokenPacketData{ | |||
packetData = transfertypes.FungibleTokenPacketData{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactor to reduce duplication in test setup.
The setup for packetData
is repeated across multiple test cases with only minor variations. Consider using a helper function to create packetData
to reduce code duplication and improve maintainability.
Also applies to: 62-62, 84-84, 103-103, 125-125, 145-145, 167-167, 187-187, 214-214, 228-228, 242-242
sender = sdk.AccAddress(secp256k1.GenPrivKey().PubKey().Address()).String() | ||
receiver = sdk.AccAddress(secp256k1.GenPrivKey().PubKey().Address()).String() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure the use of deterministic addresses for testing.
Using secp256k1.GenPrivKey().PubKey().Address()
generates a new address every time, which can lead to non-deterministic tests. Consider using fixed addresses or mocking the address generation.
packetData = transfertypes.NewFungibleTokenPacketDataV2( | ||
[]transfertypes.Token{ | ||
{ | ||
Denom: ibctesting.TestCoin.GetDenom(), | ||
Amount: ibctesting.TestCoin.Amount.String(), | ||
Trace: []string{}, | ||
}, | ||
}, | ||
ibctesting.TestAccAddress, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The creation of FungibleTokenPacketDataV2
should include tests for invalid or boundary values for tokens.
Would you like me to add these tests or should I open a GitHub issue to track this task?
@@ -53,6 +53,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ | |||
* (core) [\#6138](https://github.com/cosmos/ibc-go/pull/6138) Remove `Router` reference from IBC core keeper and use instead the router on the existing `PortKeeper` reference. | |||
* (core/04-channel) [\#6023](https://github.com/cosmos/ibc-go/pull/6023) Remove emission of non-hexlified event attributes `packet_data` and `packet_ack`. | |||
* (core) [\#6320](https://github.com/cosmos/ibc-go/pull/6320) Remove unnecessary `Proof` interface from `exported` package. | |||
* (core/05-port) [\#6341](https://github.com/cosmos/ibc-go/pull/6341) Modify `UnmarshalPacketData` interface to take in the context, portID, and channelID. This allows for packet data's to be unmarshaled based on the channel version. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove trailing spaces.
- * (core/05-port) [\#6341](https://github.com/cosmos/ibc-go/pull/6341) Modify `UnmarshalPacketData` interface to take in the context, portID, and channelID. This allows for packet data's to be unmarshaled based on the channel version.
+ * (core/05-port) [\#6341](https://github.com/cosmos/ibc-go/pull/6341) Modify `UnmarshalPacketData` interface to take in the context, portID, and channelID. This allows for packet data's to be unmarshaled based on the channel version.
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.
* (core/05-port) [\#6341](https://github.com/cosmos/ibc-go/pull/6341) Modify `UnmarshalPacketData` interface to take in the context, portID, and channelID. This allows for packet data's to be unmarshaled based on the channel version. | |
* (core/05-port) [\#6341](https://github.com/cosmos/ibc-go/pull/6341) Modify `UnmarshalPacketData` interface to take in the context, portID, and channelID. This allows for packet data's to be unmarshaled based on the channel version. |
const ( | ||
allocationNotFound = -1 | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Define the constant allocationNotFound
more clearly to avoid magic numbers.
- const (
- allocationNotFound = -1
- )
+ const allocationNotFound int = -1 // Indicates that the allocation was not found.
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.
const ( | |
allocationNotFound = -1 | |
) | |
const allocationNotFound int = -1 // Indicates that the allocation was not found. |
// tokens to be transferred | ||
repeated cosmos.base.v1beta1.Coin tokens = 9 [(gogoproto.nullable) = false, (amino.dont_omitempty) = true]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider deprecating the token
field if it is redundant due to the new tokens
field, which supports multiple tokens as per the ICS20-V2 specification.
func (im IBCMiddleware) UnmarshalPacketData(ctx sdk.Context, portID, channelID string, bz []byte) (interface{}, error) { | ||
unmarshaler, ok := im.app.(porttypes.PacketDataUnmarshaler) | ||
if !ok { | ||
return nil, errorsmod.Wrapf(types.ErrUnsupportedAction, "underlying app does not implement %T", (*porttypes.PacketDataUnmarshaler)(nil)) | ||
} | ||
|
||
return unmarshaler.UnmarshalPacketData(bz) | ||
return unmarshaler.UnmarshalPacketData(ctx, portID, channelID, bz) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure proper error handling when the underlying app does not support packet data unmarshaling.
The error message in UnmarshalPacketData
should provide more context about the failure, including the port and channel IDs involved, to aid in debugging.
- return nil, errorsmod.Wrapf(types.ErrUnsupportedAction, "underlying app does not implement %T", (*porttypes.PacketDataUnmarshaler)(nil))
+ return nil, errorsmod.Wrapf(types.ErrUnsupportedAction, "underlying app for port %s and channel %s does not implement PacketDataUnmarshaler", portID, channelID)
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.
func (im IBCMiddleware) UnmarshalPacketData(ctx sdk.Context, portID, channelID string, bz []byte) (interface{}, error) { | |
unmarshaler, ok := im.app.(porttypes.PacketDataUnmarshaler) | |
if !ok { | |
return nil, errorsmod.Wrapf(types.ErrUnsupportedAction, "underlying app does not implement %T", (*porttypes.PacketDataUnmarshaler)(nil)) | |
} | |
return unmarshaler.UnmarshalPacketData(bz) | |
return unmarshaler.UnmarshalPacketData(ctx, portID, channelID, bz) | |
func (im IBCMiddleware) UnmarshalPacketData(ctx sdk.Context, portID, channelID string, bz []byte) (interface{}, error) { | |
unmarshaler, ok := im.app.(porttypes.PacketDataUnmarshaler) | |
if !ok { | |
return nil, errorsmod.Wrapf(types.ErrUnsupportedAction, "underlying app for port %s and channel %s does not implement PacketDataUnmarshaler", portID, channelID) | |
} | |
return unmarshaler.UnmarshalPacketData(ctx, portID, channelID, bz) |
if err := packetData.ValidateBasic(); err != nil { | ||
panic(err) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using panic
for error handling in PacketDataV1ToV2
is not recommended. Consider returning the error to the caller to handle it more gracefully.
- panic(err)
+ return types.FungibleTokenPacketDataV2{}, err
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.
if err := packetData.ValidateBasic(); err != nil { | |
panic(err) | |
} | |
if err := packetData.ValidateBasic(); err != nil { | |
return types.FungibleTokenPacketDataV2{}, err | |
} |
// extractDenomAndTraceFromV1Denom extracts the base denom and remaining trace from a v1 IBC denom. | ||
func ExtractDenomAndTraceFromV1Denom(v1Denom string) (string, []string) { | ||
v1DenomTrace := types.ParseDenomTrace(v1Denom) | ||
|
||
// if the path string is empty, then the base denom is the full native denom. | ||
if v1DenomTrace.Path == "" { | ||
return v1DenomTrace.BaseDenom, nil | ||
} | ||
|
||
splitPath := strings.Split(v1DenomTrace.Path, "/") | ||
|
||
// this condition should never be reached. | ||
if len(splitPath)%2 != 0 { | ||
panic("pathSlice length is not even") | ||
} | ||
|
||
// the path slices consists of entries of ports and channel ids separately, | ||
// we need to combine them to form the trace. | ||
var trace []string | ||
for i := 0; i < len(splitPath); i += 2 { | ||
trace = append(trace, strings.Join(splitPath[i:i+2], "/")) | ||
} | ||
|
||
return v1DenomTrace.BaseDenom, trace | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tip
Codebase Verification
The test files mbt_relay_test.go
and relay_test.go
do not contain specific test cases for the edge case where the path length is not even in the function ExtractDenomAndTraceFromV1Denom
. This edge case should be explicitly tested to ensure robustness.
- mbt_relay_test.go: This file contains various tests related to token transfers but does not include tests for uneven path lengths.
- relay_test.go: Similarly, this file tests different scenarios for token transfers but lacks tests for the specific edge case of uneven path lengths.
To ensure comprehensive testing, please add test cases that specifically handle the scenario where the path length is not even.
Analysis chain
The function ExtractDenomAndTraceFromV1Denom
is well-implemented to handle the extraction of denomination and trace from a V1 denom. However, ensure that the edge cases are thoroughly tested, especially the condition where the path length is not even.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that edge cases for `ExtractDenomAndTraceFromV1Denom` are thoroughly tested.
# Test: Search for test cases that specifically test uneven path lengths. Expect: At least one test case.
rg --type go "ExtractDenomAndTraceFromV1Denom" modules/apps/transfer/internal/convert/
Length of output: 329
Script:
#!/bin/bash
# Description: Verify that edge cases for `ExtractDenomAndTraceFromV1Denom` are thoroughly tested.
# Test: Search for test cases that specifically test uneven path lengths. Expect: At least one test case.
rg --type go "ExtractDenomAndTraceFromV1Denom" --glob '*_test.go'
Length of output: 1050
Script:
#!/bin/bash
# Description: Display relevant lines from test files to verify edge case coverage for `ExtractDenomAndTraceFromV1Denom`.
# Display lines around the usage of `ExtractDenomAndTraceFromV1Denom` in the identified test files.
rg --type go "ExtractDenomAndTraceFromV1Denom" --glob '*_test.go' -A 5 -B 5
Length of output: 5833
Script:
#!/bin/bash
# Description: Display full content of test files to verify edge case coverage for `ExtractDenomAndTraceFromV1Denom`.
# Display full content of the identified test files.
cat modules/apps/transfer/keeper/mbt_relay_test.go
cat modules/apps/transfer/keeper/relay_test.go
Length of output: 51115
) | ||
}() | ||
|
||
return nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be a continue
😅. I'll open a PR and add some tests on recv to check number of packets received.
Description
ICS20-V2 Feature branch
closes: #5793
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/
).godoc
comments.Files changed
in the GitHub PR explorer.SonarCloud Report
in the comment section below once CI passes.Summary by CodeRabbit
Documentation
UnmarshalPacketData
.New Features
DefaultTransferCoins
function for easier coin handling.MsgTransfer
.Bug Fixes
MsgTransfer
tests.Refactor
MsgTransfer
initialization to usesdk.NewCoins
for better consistency.transfertypes.Version
withtransfertypes.V2
for channel versioning.Tests
MsgTransfer
parameters and multi-token scenarios.Chores
BankKeeper
interface to support variadicsdk.Coin
arguments.