feat(ibc transfer): Register IBC denom on transfer #900
Conversation
|
Please could you make and link an issue, and cross link the rdk pr? |
danwt
changed the title
|
e2e tests fail, is this the version mismatch that @omritoptix mentioned in today's standup? |
There was a problem hiding this comment.
Architecturally:
I wonder if we can do better by decoupling the memo protection from the denom metadata concepts. We have here a package called 'transferinject' which is quite abstract and general sounding but it only specifically supports this denom metadata memo stuff
Happy to merge like that and improve later though
Security and tests
Need to test and define correct behavior for
- memo not included and denom meta not included (happy A)
- memo not included and denom meta already sent and acked (happy B)
2.5) memo not included and denom meta already sent but not acked - memo included and denom meta not included (attack)
- memo included and denom meta already sent and acked (attack)
etc
|
also please stop adding 'failed to' etc to errors 😄 |
|
Btw does any proto for old messages/tx need to be deprecated? And the corresponding generated code? |
…stom-ibc-transfer-packet
ikr, it's a habit. Probably coming from some baggage from my upbringing. "I'm not only an error, but also a failure" 😭 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
x/rollapp/types/rollapp.pb.gois excluded by!**/*.pb.go
Files selected for processing (24)
- CHANGELOG.md (1 hunks)
- app/app.go (8 hunks)
- ibctesting/delayed_ack_test.go (2 hunks)
- ibctesting/eibc_test.go (4 hunks)
- ibctesting/utils_test.go (2 hunks)
- proto/dymension/rollapp/rollapp.proto (1 hunks)
- testutil/keeper/delayedack.go (2 hunks)
- testutil/keeper/rollapp.go (2 hunks)
- x/delayedack/eibc.go (3 hunks)
- x/delayedack/ibc_middleware.go (16 hunks)
- x/delayedack/keeper/fraud_test.go (2 hunks)
- x/delayedack/keeper/invariants_test.go (2 hunks)
- x/delayedack/keeper/keeper.go (6 hunks)
- x/delayedack/keeper/params_test.go (1 hunks)
- x/delayedack/rollapp_hooks.go (2 hunks)
- x/delayedack/types/expected_keepers.go (2 hunks)
- x/eibc/genesis_test.go (1 hunks)
- x/rollapp/keeper/keeper.go (4 hunks)
- x/rollapp/keeper/util.go (1 hunks)
- x/transferinject/ibc_middleware.go (1 hunks)
- x/transferinject/ibc_middleware_test.go (1 hunks)
- x/transferinject/types/codec.go (1 hunks)
- x/transferinject/types/expected_keepers.go (1 hunks)
- x/transferinject/types/packet_metadata.go (1 hunks)
Files skipped from review due to trivial changes (4)
- testutil/keeper/rollapp.go
- x/delayedack/keeper/params_test.go
- x/eibc/genesis_test.go
- x/transferinject/types/codec.go
Additional context used
LanguageTool
CHANGELOG.md
[uncategorized] ~85-~85: Do not mix variants of the same word (‘acknowledgement’ and ‘acknowledgment’) within a single text. (EN_WORD_COHERENCY)
Context: ....com//issues/822) Acknowledgement not written in case of ackerr - (rollap...
Additional comments not posted (29)
x/transferinject/types/expected_keepers.go (1)
16-22: Confirm the usage ofExtractRollappFromChannelmethod.Verification successful
The
ExtractRollappFromChannelmethod is consistently implemented and used across the codebase. Here are the key points:
- The method is implemented in
x/rollapp/keeper/util.goand correctly matches the interface definition.- It is used in
x/transferinject/ibc_middleware.goto handle IBC transfers.- A mock implementation is provided in
x/transferinject/ibc_middleware_test.gofor testing purposes.No issues were found with the addition of this method.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `ExtractRollappFromChannel` method across the codebase. # Test: Search for the method usage. Expect: Consistent usage across the codebase. rg --type go $'ExtractRollappFromChannel'Length of output: 831
Script:
#!/bin/bash # Extract the implementation of ExtractRollappFromChannel in util.go ast-grep --lang go --pattern $'func (k Keeper) ExtractRollappFromChannel($_, $_, $_) ($_, $_) { $$$ }' x/rollapp/keeper/util.go # Extract the usage of ExtractRollappFromChannel in ibc_middleware.go rg --context 5 --type go $'ExtractRollappFromChannel' x/transferinject/ibc_middleware.go # Extract the usage of ExtractRollappFromChannel in ibc_middleware_test.go rg --context 5 --type go $'ExtractRollappFromChannel' x/transferinject/ibc_middleware_test.goLength of output: 3146
x/delayedack/rollapp_hooks.go (2)
19-19: Ensure correct logic inAfterStateFinalized.Verification successful
Ensure correct logic in
AfterStateFinalized.
Ensure correct logic in
AfterStateFinalized.
The
AfterStateFinalizedmethod inx/delayedack/rollapp_hooks.gocorrectly finalizes the rollapp packets by calculating thestateEndHeightand callingim.FinalizeRollappPackets. The method appears to interact properly with other components based on the extracted implementation.
x/delayedack/rollapp_hooks.go:16-20: Implementation ofAfterStateFinalized.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the logic in `AfterStateFinalized` method. # Test: Search for the method usage and related logic. Expect: Correct interaction with other components. rg --type go $'AfterStateFinalized'Length of output: 1035
Script:
#!/bin/bash # Description: Verify the implementation details of `AfterStateFinalized` method in `x/delayedack/rollapp_hooks.go`. # Extract the implementation of `AfterStateFinalized` method. ast-grep --lang go --pattern $'func (im IBCMiddleware) AfterStateFinalized(ctx sdk.Context, rollappID string, stateInfo *rollapptypes.StateInfo) error { $$$ }' > after_state_finalized_impl.txt # Extract the usage of `AfterStateFinalized` method in other files. rg --type go 'AfterStateFinalized' > after_state_finalized_usage.txtLength of output: 197
Script:
#!/bin/bash # Description: Verify the implementation details of `AfterStateFinalized` method in `x/delayedack/rollapp_hooks.go`. # Extract the implementation of `AfterStateFinalized` method. ast-grep --lang go --pattern $'func (im IBCMiddleware) AfterStateFinalized(ctx sdk.Context, rollappID string, stateInfo *rollapptypes.StateInfo) error { $$$ }' x/delayedack/rollapp_hooks.go # Extract the usage of `AfterStateFinalized` method in other files. rg --type go 'AfterStateFinalized' > after_state_finalized_usage.txtLength of output: 724
23-23: Confirm the handling of fraud detection inFraudSubmitted.Verification successful
The
HandleFraudmethod inx/delayedack/keeper/fraud.gocorrectly handles fraud detection by reverting pending IBC rollapp packets and updating their status to reverted. This confirms that theFraudSubmittedmethod inx/delayedack/rollapp_hooks.goappropriately handles fraud detection by callingHandleFraud.
x/delayedack/keeper/fraud.go:
- The
HandleFraudmethod retrieves pending packets, logs the fraud event, iterates over the packets to revert them, and updates their status to reverted.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the handling of fraud detection in `FraudSubmitted` method. # Test: Search for the method usage and related logic. Expect: Correct fraud detection mechanisms. rg --type go $'FraudSubmitted'Length of output: 1062
Script:
#!/bin/bash # Extract the definition and implementation of FraudSubmitted in x/delayedack/rollapp_hooks.go ast-grep --lang go --pattern $'func (im IBCMiddleware) FraudSubmitted(ctx sdk.Context, rollappID string, height uint64, seqAddr string) error { $$$ }' x/delayedack/rollapp_hooks.go # Extract the definition and implementation of FraudSubmitted in x/rollapp/types/hooks.go ast-grep --lang go --pattern $'func (h MultiRollappHooks) FraudSubmitted(ctx sdk.Context, rollappID string, height uint64, seqAddr string) error { $$$ }' x/rollapp/types/hooks.go # Extract the usage of FraudSubmitted in x/rollapp/keeper/fraud_handler.go rg --context 5 'FraudSubmitted' x/rollapp/keeper/fraud_handler.go # Extract the implementation of FraudSubmitted in x/sequencer/keeper/hook_listener.go ast-grep --lang go --pattern $'func (hook rollapphook) FraudSubmitted(ctx sdk.Context, rollappID string, height uint64, seqAddr string) error { $$$ }' x/sequencer/keeper/hook_listener.go # Extract the test cases for FraudSubmitted in x/sequencer/keeper/hooks_test.go rg --context 5 'FraudSubmitted' x/sequencer/keeper/hooks_test.goLength of output: 3631
Script:
#!/bin/bash # Extract the definition and implementation of HandleFraud in x/delayedack/rollapp_hooks.go ast-grep --lang go --pattern $'func (im IBCMiddleware) HandleFraud(ctx sdk.Context, rollappID string, module string) error { $$$ }' x/delayedack/rollapp_hooks.goLength of output: 164
Script:
#!/bin/bash # Search for the definition of HandleFraud in the entire codebase rg 'func (im IBCMiddleware) HandleFraud' --type goLength of output: 52
Script:
#!/bin/bash # Search for any method containing the term Fraud in the entire codebase rg 'func .*Fraud' --type goLength of output: 4257
Script:
#!/bin/bash # Extract the definition and implementation of HandleFraud in x/delayedack/keeper/fraud.go ast-grep --lang go --pattern $'func (k Keeper) HandleFraud(ctx sdk.Context, rollappID string, ibc porttypes.IBCModule) error { $$$ }' x/delayedack/keeper/fraud.goLength of output: 3006
x/transferinject/types/packet_metadata.go (3)
12-23: Confirm the structure and validation logic ofTransferInject.Verification successful
The
TransferInjectstructure and its validation logic appear to be correctly defined and used in the codebase. TheValidateBasicmethod callsValidateon theDenomMetadatafield, which is consistent with the expected behavior. Additionally, the references toTransferInjectin error handling and parsing packet metadata indicate that it is integrated into the codebase as intended.
TransferInjectstructure is defined with aDenomMetadatafield.ValidateBasicmethod callsValidateonDenomMetadata.TransferInjectis used in error handling and parsing packet metadata.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the structure and validation logic of `TransferInject`. # Test: Search for the structure usage and validation logic. Expect: Correct structure definition and validation. rg --type go $'TransferInject'Length of output: 1610
58-74: Confirm the logic for adding denom metadata to memo inAddDenomMetadataToMemo.Verification successful
The logic for adding denom metadata to memo in
AddDenomMetadataToMemoappears to be correctly implemented and integrated based on the following observations:
Implementation in
packet_metadata.go:
- The function checks if the memo already contains a transfer inject object and returns an error if it does.
- It adds the denom metadata to the memo and marshals it back to a string.
Usage in
ibc_middleware_test.go:
- The helper function
addDenomMetadataToPacketDatausesAddDenomMetadataToMemoto add metadata to the memo, indicating that the function is tested.Usage in
ibc_middleware.go:
- The function is used to add metadata to the packet memo before sending it, and errors are appropriately handled.
The logic for adding denom metadata to memo in
AddDenomMetadataToMemois verified to be correct.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the logic for adding denom metadata to memo in `AddDenomMetadataToMemo`. # Test: Search for the function usage and related logic. Expect: Correct metadata addition. rg --type go $'AddDenomMetadataToMemo'Length of output: 376
Script:
#!/bin/bash # Description: Review the usage of AddDenomMetadataToMemo in ibc_middleware_test.go and ibc_middleware.go # Extract the relevant lines from ibc_middleware_test.go rg -A 10 -B 10 'AddDenomMetadataToMemo' x/transferinject/ibc_middleware_test.go # Extract the relevant lines from ibc_middleware.go rg -A 10 -B 10 'AddDenomMetadataToMemo' x/transferinject/ibc_middleware.goLength of output: 1638
32-45: Ensure correct error handling inParsePacketMetadataandMemoAlreadyHasPacketMetadata.Verification successful
The error handling in
ParsePacketMetadataandMemoAlreadyHasPacketMetadatafunctions is correctly implemented.
- In
x/transferinject/ibc_middleware.go, errors fromParsePacketMetadataare checked and handled appropriately.- In
x/delayedack/eibc.go, specific errors fromParsePacketMetadataare checked and handled.- The return value of
MemoAlreadyHasPacketMetadatais checked, and an error is returned if the memo already has packet metadata.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the error handling in `ParsePacketMetadata` and `MemoAlreadyHasPacketMetadata`. # Test: Search for the functions usage and related error handling. Expect: Proper error handling mechanisms. rg --type go $'ParsePacketMetadata|MemoAlreadyHasPacketMetadata'Length of output: 733
Script:
#!/bin/bash # Description: Verify the error handling in the usage of `ParsePacketMetadata`. # Check error handling in `x/transferinject/ibc_middleware.go` rg -A 5 'ParsePacketMetadata' x/transferinject/ibc_middleware.go # Check error handling in `x/delayedack/eibc.go` rg -A 5 'ParsePacketMetadata' x/delayedack/eibc.goLength of output: 576
Script:
#!/bin/bash # Description: Verify the usage of `MemoAlreadyHasPacketMetadata`. # Check usage in `x/transferinject/ibc_middleware.go` rg -A 5 'MemoAlreadyHasPacketMetadata' x/transferinject/ibc_middleware.goLength of output: 302
x/delayedack/types/expected_keepers.go (1)
41-46: Confirm the usage ofExtractRollappIDAndTransferPacketFromDatamethod.Verification successful
The method
ExtractRollappIDAndTransferPacketFromDatais implemented inx/rollapp/keeper/util.goand is used inx/delayedack/ibc_middleware.goandtestutil/keeper/delayedack.go. The implementation aligns with its intended purpose, and the usage appears consistent across the codebase.
Implementation in
x/rollapp/keeper/util.go:func (k Keeper) ExtractRollappIDAndTransferPacketFromData( ctx sdk.Context, data []byte, rollappPortOnHub string, rollappChannelOnHub string, ) (string, *transfertypes.FungibleTokenPacketData, error) { // no-op if the packet is not a fungible token packet packet := new(transfertypes.FungibleTokenPacketData) if err := types.ModuleCdc.UnmarshalJSON(data, packet); err != nil { return "", packet, errorsmod.Wrapf(err, "unmarshal packet data") } rollapp, err := k.ExtractRollappFromChannel(ctx, rollappPortOnHub, rollappChannelOnHub) if err != nil { return "", packet, err } if rollapp == nil { return "", packet, nil } return rollapp.RollappId, packet, nil }Usage in
x/delayedack/ibc_middleware.go:rollappID, transferPacketData, err := im.rollappKeeper.ExtractRollappIDAndTransferPacketFromData( ctx, packet.Data, rollappPortOnHub, rollappChannelOnHub)Usage in
testutil/keeper/delayedack.go:func (r RollappKeeperStub) ExtractRollappIDAndTransferPacketFromData(sdk.Context, []byte, string, string) (string, *transfertypes.FungibleTokenPacketData, error) { // Stub implementation }The method is correctly implemented and used consistently across the codebase.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `ExtractRollappIDAndTransferPacketFromData` method across the codebase. # Test: Search for the method usage. Expect: Consistent usage across the codebase. rg --type go $'ExtractRollappIDAndTransferPacketFromData'Length of output: 997
Script:
#!/bin/bash # Description: Verify the implementation and usage of `ExtractRollappIDAndTransferPacketFromData`. # Test: Retrieve the implementation of the method in `x/rollapp/keeper/util.go`. ast-grep --lang go --pattern $'func (k Keeper) ExtractRollappIDAndTransferPacketFromData($_, $_, $_, $_) ($_ , $_, $_) { $$$ }' --json # Test: Retrieve the usage of the method in `x/delayedack/ibc_middleware.go`. rg --type go $'ExtractRollappIDAndTransferPacketFromData' x/delayedack/ibc_middleware.go # Test: Retrieve the usage of the method in `testutil/keeper/delayedack.go`. rg --type go $'ExtractRollappIDAndTransferPacketFromData' testutil/keeper/delayedack.goLength of output: 2581
proto/dymension/rollapp/rollapp.proto (1)
44-45: Confirm the usage ofregisteredDenomsfield inRollappmessage.Verification successful
The
registeredDenomsfield in theRollappmessage is consistently used across the codebase, including in the Go files. The field is integrated and utilized correctly in various parts of the system.
proto/dymension/rollapp/rollapp.proto: Field definition.x/rollapp/types/rollapp.pb.go: Field usage and related methods.x/transferinject/ibc_middleware.go: Field usage in middleware logic.x/transferinject/ibc_middleware_test.go: Field usage in tests.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `registeredDenoms` field in `Rollapp` message across the codebase. # Test: Search for the field usage. Expect: Consistent usage across the codebase. rg --type proto $'registeredDenoms'Length of output: 64
Script:
#!/bin/bash # Description: Verify the usage of `registeredDenoms` field in `Rollapp` message across the codebase. # Test: Search for the field usage. Expect: Consistent usage across the codebase. rg 'registeredDenoms'Length of output: 449
Script:
#!/bin/bash # Description: Verify the usage of `registeredDenoms` field in Go code. # Test: Search for the field usage in Go files. Expect: Consistent usage across the codebase. rg 'RegisteredDenoms' --type goLength of output: 1845
x/rollapp/keeper/util.go (2)
14-38: Ensure correct implementation ofExtractRollappIDAndTransferPacketFromData.Verification successful
The function
ExtractRollappIDAndTransferPacketFromDatais used in multiple contexts within the codebase, includingx/delayedack/ibc_middleware.go,x/delayedack/types/expected_keepers.go, andtestutil/keeper/delayedack.go. The usages appear to be consistent with the function's purpose of extracting rollapp ID and fungible token packet data.
- In
x/delayedack/ibc_middleware.go, the function is used to extract rollapp ID and transfer packet data from the packet data.- In
x/delayedack/types/expected_keepers.go, the function is declared as part of the expected keeper interface.- In
testutil/keeper/delayedack.go, a stub implementation of the function is provided for testing purposes.These usages indicate that the function is correctly integrated and utilized within the codebase.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of `ExtractRollappIDAndTransferPacketFromData`. # Test: Search for the function usage and related logic. Expect: Correct implementation and interaction with other components. rg --type go $'ExtractRollappIDAndTransferPacketFromData'Length of output: 997
Script:
#!/bin/bash # Description: Verify the context and correctness of `ExtractRollappIDAndTransferPacketFromData` usage. # Check the context in `x/delayedack/ibc_middleware.go` echo "Context in x/delayedack/ibc_middleware.go:" rg --context 5 --type go $'ExtractRollappIDAndTransferPacketFromData' x/delayedack/ibc_middleware.go # Check the context in `x/delayedack/types/expected_keepers.go` echo "Context in x/delayedack/types/expected_keepers.go:" rg --context 5 --type go $'ExtractRollappIDAndTransferPacketFromData' x/delayedack/types/expected_keepers.go # Check the context in `testutil/keeper/delayedack.go` echo "Context in testutil/keeper/delayedack.go:" rg --context 5 --type go $'ExtractRollappIDAndTransferPacketFromData' testutil/keeper/delayedack.goLength of output: 2854
40-70: Confirm the logic and error handling inExtractRollappFromChannel.Verification successful
To verify the logic and error handling in the
ExtractRollappFromChannelfunction and its usage in other files, we need to inspect the relevant code sections. Let's start by examining the function itself and then proceed to check its usage in the identified files.
The logic and error handling in the
ExtractRollappFromChannelfunction appear to be well-defined, with multiple checks and appropriate error returns. The function is used in several places, and the error handling in these usages also seems to be correctly implemented.Key Points:
- Function Definition: The
ExtractRollappFromChannelfunction has multiple error checks and returns appropriate errors ornilvalues based on the conditions.- Usage in
x/transferinject/types/expected_keepers.go: The function is declared in an interface, which is standard and does not involve error handling.- Usage in
x/transferinject/ibc_middleware_test.go: The function is mocked for testing purposes, returning predefined values.- Usage in
x/transferinject/ibc_middleware.go: The function is used twice, and in both cases, errors are appropriately wrapped and returned.Conclusion:
The logic and error handling in the
ExtractRollappFromChannelfunction and its usage in other files are correctly implemented.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the logic and error handling in `ExtractRollappFromChannel`. # Test: Search for the function usage and related logic. Expect: Correct error handling and logic. rg --type go $'ExtractRollappFromChannel'Length of output: 831
Script:
#!/bin/bash # Description: Verify the logic and error handling in `ExtractRollappFromChannel` and its usage in other files. # Extract the `ExtractRollappFromChannel` function definition for detailed inspection. ast-grep --lang go --pattern $'func (k Keeper) ExtractRollappFromChannel($_, $_, $_) ($_, $_) { $$$ }' x/rollapp/keeper/util.go # Extract the usage of `ExtractRollappFromChannel` in `x/transferinject/types/expected_keepers.go`. rg --context 5 --type go 'ExtractRollappFromChannel' x/transferinject/types/expected_keepers.go # Extract the usage of `ExtractRollappFromChannel` in `x/transferinject/ibc_middleware_test.go`. rg --context 5 --type go 'ExtractRollappFromChannel' x/transferinject/ibc_middleware_test.go # Extract the usage of `ExtractRollappFromChannel` in `x/transferinject/ibc_middleware.go`. rg --context 5 --type go 'ExtractRollappFromChannel' x/transferinject/ibc_middleware.goLength of output: 3547
x/delayedack/keeper/fraud_test.go (2)
13-14: Changes align with the new architecture requiringrollappKeeper.
50-51: Changes align with the new architecture requiringrollappKeeper.testutil/keeper/delayedack.go (1)
118-120: Addition ofExtractRollappIDAndTransferPacketFromDatacorrectly mirrors changes in theRollappKeeperinterface.x/rollapp/keeper/keeper.go (1)
162-168: Addition ofSetTransferKeeperis crucial for setting theTransferKeeper. Consider adding a comment to clarify that this is a required call, as suggested in a previous review.x/delayedack/eibc.go (1)
46-48: Enhancements in error handling and security checks improve the robustness and security of eIBC demand order handling.Also applies to: 67-67, 89-89
x/transferinject/ibc_middleware.go (1)
19-97: Addition ofIBCSendMiddlewareandIBCAckMiddlewareis crucial for the new feature of injecting metadata into IBC packets.Also applies to: 99-165
x/delayedack/keeper/keeper.go (3)
19-19: Added import forlogfromtendermint/tendermint/libs.This import is necessary for logging within the keeper functions, enhancing debuggability and traceability.
33-35: AddedrollappKeeperandsequencerKeeperto theKeeperstruct.This change aligns with the PR's objective to integrate
rollappKeeperfor managing rollapp-related operations andsequencerKeeperfor handling sequencer-specific logic.
67-67: Refactoredics4WrappertoICS4Wrapper.This change improves the consistency of naming conventions in the codebase, aligning with Go's convention of using CamelCase for exported identifiers.
x/delayedack/keeper/invariants_test.go (1)
18-19: LGTM! Ensure that the newrollappKeeperparameter is integrated correctly throughout the codebase.Verification successful
The
NewIBCMiddlewarefunction is used with therollappKeeperparameter in all instances found. Let's verify the correctness of therollappKeeperparameter usage in theapp/app.gofile to ensure it is integrated correctly.
The
rollappKeeperparameter is correctly integrated into theNewIBCMiddlewarefunction call in theapp/app.gofile. The parameter is used consistently across all instances found in the codebase.
x/delayedack/keeper/invariants_test.gox/delayedack/keeper/fraud_test.goapp/app.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the new `rollappKeeper` parameter is used correctly in all relevant function calls. # Test: Search for the function usage. Expect: Only occurances of the new signature. rg --type go $'NewIBCMiddleware'Length of output: 960
Script:
#!/bin/bash # Description: Verify the usage of the `rollappKeeper` parameter in the `app/app.go` file. # Test: Display the relevant lines in the `app/app.go` file. rg --type go --context 5 $'NewIBCMiddleware' app/app.goLength of output: 863
ibctesting/delayed_ack_test.go (1)
29-31: LGTM! Ensure that the denom metadata set here is utilized correctly in the tests.Verification successful
The denom metadata set by
SetDenomMetaDatainSetupTestis indeed utilized in the tests withinibctesting/delayed_ack_test.go. The metadata forsdk.DefaultBondDenomis referenced multiple times, ensuring that the setup is correctly influencing the test cases.
ibctesting/delayed_ack_test.go: The metadata forsdk.DefaultBondDenomis used in various test assertions and operations.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the denom metadata set in `SetupTest` is utilized correctly in the tests. # Test: Search for the metadata usage in tests. Expect: Metadata should be utilized in relevant tests. rg --type go $'SetDenomMetaData'Length of output: 1247
Script:
#!/bin/bash # Description: Verify that the denom metadata set in `SetupTest` is utilized correctly in the tests. # Test: Search for the usage of the metadata set by `SetDenomMetaData` in the relevant test files. rg --type go 'sdk.DefaultBondDenom' ibctesting/delayed_ack_test.goLength of output: 834
x/delayedack/ibc_middleware.go (2)
23-32: LGTM! The integration ofrollappKeeperin theIBCMiddlewarestruct and its constructor is well-implemented.
Line range hint
43-102: Ensure that the newrollappKeepermethods are used correctly in packet handling.Also applies to: 124-191, 216-282
Verification successful
To ensure the new
rollappKeepermethods are used correctly in packet handling, we need to verify the definitions and usages of the following methods:ExtractRollappIDAndTransferPacketFromData,ValidateRollappId,GetProofHeight, andCheckIfFinalized.
The methods
ExtractRollappIDAndTransferPacketFromData,ValidateRollappId,GetProofHeight, andCheckIfFinalizedare defined and used in the relevant files. The definitions and usages align with the expected functionality in the packet handling process.
ExtractRollappIDAndTransferPacketFromDatais defined inx/rollapp/keeper/util.goand used inx/delayedack/ibc_middleware.go.ValidateRollappIdis defined inx/delayedack/keeper/keeper.goand used inx/delayedack/ibc_middleware.go.GetProofHeightis defined inx/delayedack/ibc_middleware.goand used in the same file.CheckIfFinalizedis defined inx/delayedack/ibc_middleware.goand used in the same file.The methods are used correctly in the packet handling functions, ensuring proper extraction, validation, proof height retrieval, and finalization checks.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the new `rollappKeeper` methods are used correctly in packet handling functions. # Test: Search for the usage of `rollappKeeper` methods in packet handling. Expect: Methods should be used correctly. rg --type go $'rollappKeeper'Length of output: 7749
Script:
#!/bin/bash # Description: Verify the correct usage of `rollappKeeper` methods in packet handling functions. # Search for the definition and usage of `ExtractRollappIDAndTransferPacketFromData` rg --type go 'ExtractRollappIDAndTransferPacketFromData' # Search for the definition and usage of `ValidateRollappId` rg --type go 'ValidateRollappId' # Search for the definition and usage of `GetProofHeight` rg --type go 'GetProofHeight' # Search for the definition and usage of `CheckIfFinalized` rg --type go 'CheckIfFinalized'Length of output: 3150
ibctesting/utils_test.go (1)
27-27: LGTM! The update to useeibctypes.ModuleCdcfor unmarshalling is correctly implemented.Verification successful
The
eibctypes.ModuleCdc.UnmarshalJSONmethod is used in two locations:ibctesting/utils_test.goandibctesting/eibc_test.go. Let's verify the correctness of its usage in these files.
The
eibctypes.ModuleCdc.UnmarshalJSONmethod is used correctly in bothibctesting/utils_test.goandibctesting/eibc_test.go. The method is used to unmarshal JSON data into thetransfertypes.FungibleTokenPacketDatastruct, and error handling is appropriately implemented.
ibctesting/utils_test.go: The method is used within theGetRollappToHubIBCDenomFromPacketfunction, and the error is checked usingsuite.Require().NoError(err).ibctesting/eibc_test.go: The method is used to unmarshal data fromrollappPacket.Packet.GetData(), and the error is checked usingsuite.Require().NoError(err).Both usages are consistent with the expected behavior for unmarshalling JSON data.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the `eibctypes.ModuleCdc.UnmarshalJSON` method is used correctly in the tests. # Test: Search for the usage of `eibctypes.ModuleCdc.UnmarshalJSON`. Expect: Method should be used correctly. rg --type go $'eibctypes.ModuleCdc.UnmarshalJSON'Length of output: 246
Script:
#!/bin/bash # Description: Display the context around the usage of `eibctypes.ModuleCdc.UnmarshalJSON` in the identified files. # Display the relevant lines in `ibctesting/utils_test.go` rg --type go --context 5 'eibctypes.ModuleCdc.UnmarshalJSON' ibctesting/utils_test.go # Display the relevant lines in `ibctesting/eibc_test.go` rg --type go --context 5 'eibctypes.ModuleCdc.UnmarshalJSON' ibctesting/eibc_test.goLength of output: 1214
CHANGELOG.md (1)
51-51: LGTM! The changelog correctly summarizes the changes made in this PR.app/app.go (4)
622-633: Ensure correct integration ofNewIBCSendMiddlewareinTransferKeeper.The integration of
NewIBCSendMiddlewarewithTransferKeeperappears correctly implemented. This middleware is crucial for handling IBC send operations with additional logic for denom registration, aligning with the PR's objectives.
754-755: Review the integration ofNewIBCAckMiddlewarewithdelayedAckMiddleware.The chaining of
NewIBCAckMiddlewarewithdelayedAckMiddlewareis correctly implemented. This setup is essential for handling acknowledgments in the IBC middleware stack, ensuring that the Rollapp's state is correctly updated upon packet acknowledgment.
635-635: ConfirmSetTransferKeeperusage.Verification successful
The
SetTransferKeepermethod is correctly implemented inx/rollapp/keeper/keeper.goand is used appropriately inapp/app.go.
- The method ensures that the
transferKeeperis not set twice by checking if it is already set and panicking if it is.- The usage in
app/app.gois consistent with the method's purpose, setting theTransferKeeperforRollappKeeper.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if `SetTransferKeeper` is used correctly across the codebase. # Test: Search for `SetTransferKeeper` usage. Expect: Correct and consistent usage across the codebase. rg --type go $'SetTransferKeeper'Length of output: 282
Script:
#!/bin/bash # Description: Verify the implementation of `SetTransferKeeper` in `x/rollapp/keeper/keeper.go` and its usage in `app/app.go`. # Extract the implementation of `SetTransferKeeper` from `x/rollapp/keeper/keeper.go` ast-grep --lang go --pattern $'func (k *Keeper) SetTransferKeeper($_) { $$$ }' --json # Extract the relevant code around the usage of `SetTransferKeeper` in `app/app.go` rg --context 5 'SetTransferKeeper' app/app.goLength of output: 1054
767-767: Clarify the role ofdelayedAckMiddlewareinSetHooks.Verification successful
The
delayedAckMiddlewareis instantiated using theNewIBCMiddlewarefunction from thedelayedackmodulepackage, which initializes anIBCMiddlewarestruct withIBCModule,Keeper, androllappKeeper. This middleware is then passed to theNewIBCAckMiddlewarefunction from thetransferinjectpackage, which initializes anIBCAckMiddlewarestruct withIBCModuleandrollappKeeper.
delayedAckMiddlewareis created inapp/app.gousingdelayedackmodule.NewIBCMiddleware.- It is then used in
transferinject.NewIBCAckMiddleware.The role of
delayedAckMiddlewareinSetHooksis to act as a middleware component in the IBC (Inter-Blockchain Communication) stack, likely handling delayed acknowledgments in the communication process.
delayedackmodule.NewIBCMiddlewaredefinition:./x/delayedack/ibc_middleware.go:28transferinject.NewIBCAckMiddlewaredefinition:./x/transferinject/ibc_middleware.go:108Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the role and impact of `delayedAckMiddleware` in the system. # Test: Search for `delayedAckMiddleware` usage and its impact on system behavior. Expect: Clear understanding and documentation of its role. rg --type go $'delayedAckMiddleware'Length of output: 300
Script:
#!/bin/bash # Locate the definition of `NewIBCMiddleware` in the `delayedackmodule` package ast-grep --lang go --pattern 'func NewIBCMiddleware($_, $_, $_) $_' # Locate the definition of `NewIBCAckMiddleware` in the `transferinject` package ast-grep --lang go --pattern 'func NewIBCAckMiddleware($_, $_) $_' # Find all usages of `delayedAckMiddleware` in the codebase to understand its context rg 'delayedAckMiddleware'Length of output: 1463
ibctesting/eibc_test.go
Outdated
| // // | ||
| // Transfer initial IBC funds to fulfiller account with ibc memo, to give him some funds to use to fulfill stuff | ||
| //// | ||
| // // |
There was a problem hiding this comment.
Remove commented-out code.
Commented-out code can clutter the codebase and make maintenance harder. If this code is not needed, it should be removed to keep the codebase clean.
| }, | ||
| wantSentData: types.ModuleCdc.MustMarshalJSON(&transfertypes.FungibleTokenPacketData{ | ||
| Denom: "adym", | ||
| Memo: addDenomMetadataToPacketData("thanks for the sweater, grandma!", validDenomMetadata), | ||
| }), | ||
| }, { | ||
| name: "error: denom metadata already in memo", | ||
| fields: fields{ | ||
| ICS4Wrapper: &mockICS4Wrapper{}, | ||
| }, | ||
| data: &transfertypes.FungibleTokenPacketData{ | ||
| Denom: "adym", | ||
| Memo: `{"transferinject":{}}`, | ||
| }, | ||
| wantSentData: []byte(""), | ||
| wantErr: types.ErrMemoTransferInjectAlreadyExists, | ||
| }, { | ||
| name: "error: extract rollapp from channel", | ||
| fields: fields{ | ||
| ICS4Wrapper: &mockICS4Wrapper{}, | ||
| rollappKeeper: &mockRollappKeeper{ | ||
| err: errortypes.ErrInvalidRequest, | ||
| }, | ||
| }, | ||
| data: &transfertypes.FungibleTokenPacketData{ | ||
| Denom: "adym", | ||
| }, | ||
| wantSentData: []byte(""), | ||
| wantErr: errortypes.ErrInvalidRequest, | ||
| }, { | ||
| name: "send unaltered: rollapp not found", | ||
| fields: fields{ | ||
| ICS4Wrapper: &mockICS4Wrapper{}, | ||
| rollappKeeper: &mockRollappKeeper{}, | ||
| }, | ||
| data: &transfertypes.FungibleTokenPacketData{ | ||
| Denom: "adym", | ||
| Memo: "user memo", | ||
| }, | ||
| wantSentData: types.ModuleCdc.MustMarshalJSON(&transfertypes.FungibleTokenPacketData{ | ||
| Denom: "adym", | ||
| Memo: "user memo", | ||
| }), | ||
| }, { | ||
| name: "send unaltered: denom metadata already in rollapp", | ||
| fields: fields{ | ||
| ICS4Wrapper: &mockICS4Wrapper{}, | ||
| rollappKeeper: &mockRollappKeeper{ | ||
| returnRollapp: &rollapptypes.Rollapp{ | ||
| RegisteredDenoms: []string{"adym"}, | ||
| }, | ||
| }, | ||
| }, | ||
| data: &transfertypes.FungibleTokenPacketData{ | ||
| Denom: "adym", | ||
| }, | ||
| wantSentData: types.ModuleCdc.MustMarshalJSON(&transfertypes.FungibleTokenPacketData{ | ||
| Denom: "adym", | ||
| }), | ||
| }, { | ||
| name: "error: get denom metadata", | ||
| fields: fields{ | ||
| ICS4Wrapper: &mockICS4Wrapper{}, | ||
| rollappKeeper: &mockRollappKeeper{ | ||
| returnRollapp: &rollapptypes.Rollapp{}, | ||
| }, | ||
| bankKeeper: mockBankKeeper{}, | ||
| }, | ||
| data: &transfertypes.FungibleTokenPacketData{ | ||
| Denom: "adym", | ||
| }, | ||
| wantSentData: types.ModuleCdc.MustMarshalJSON(&transfertypes.FungibleTokenPacketData{ | ||
| Denom: "adym", | ||
| }), | ||
| }, | ||
| } | ||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| m := transferinject.NewIBCSendMiddleware(tt.fields.ICS4Wrapper, tt.fields.rollappKeeper, tt.fields.bankKeeper) | ||
|
|
||
| data := types.ModuleCdc.MustMarshalJSON(tt.data) | ||
|
|
||
| _, err := m.SendPacket( | ||
| sdk.Context{}, | ||
| &capabilitytypes.Capability{}, | ||
| "transfer", | ||
| "channel-0", | ||
| clienttypes.Height{}, | ||
| 0, | ||
| data, | ||
| ) | ||
| if tt.wantErr == nil { | ||
| require.NoError(t, err) | ||
| } else { | ||
| require.ErrorIs(t, err, tt.wantErr) | ||
| } | ||
| require.Equal(t, string(tt.wantSentData), string(tt.fields.ICS4Wrapper.(*mockICS4Wrapper).sentData)) | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Ensure error handling uses errors.Is for consistency and clarity.
- require.ErrorIs(t, err, tt.wantErr)
+ require.True(t, errors.Is(err, tt.wantErr))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 TestIBCMiddleware_SendPacket(t *testing.T) { | |
| type fields struct { | |
| ICS4Wrapper porttypes.ICS4Wrapper | |
| rollappKeeper types.RollappKeeper | |
| bankKeeper types.BankKeeper | |
| } | |
| tests := []struct { | |
| name string | |
| fields fields | |
| data *transfertypes.FungibleTokenPacketData | |
| wantSentData []byte | |
| wantErr error | |
| }{ | |
| { | |
| name: "success: added denom metadata to memo", | |
| fields: fields{ | |
| ICS4Wrapper: &mockICS4Wrapper{}, | |
| rollappKeeper: &mockRollappKeeper{ | |
| returnRollapp: &rollapptypes.Rollapp{}, | |
| }, | |
| bankKeeper: mockBankKeeper{ | |
| returnMetadata: validDenomMetadata, | |
| }, | |
| }, | |
| data: &transfertypes.FungibleTokenPacketData{ | |
| Denom: "adym", | |
| }, | |
| wantSentData: types.ModuleCdc.MustMarshalJSON(&transfertypes.FungibleTokenPacketData{ | |
| Denom: "adym", | |
| Memo: addDenomMetadataToPacketData("", validDenomMetadata), | |
| }), | |
| }, { | |
| name: "success: added denom metadata to non-empty user memo", | |
| fields: fields{ | |
| ICS4Wrapper: &mockICS4Wrapper{}, | |
| rollappKeeper: &mockRollappKeeper{ | |
| returnRollapp: &rollapptypes.Rollapp{}, | |
| }, | |
| bankKeeper: mockBankKeeper{ | |
| returnMetadata: validDenomMetadata, | |
| }, | |
| }, | |
| data: &transfertypes.FungibleTokenPacketData{ | |
| Denom: "adym", | |
| Memo: "thanks for the sweater, grandma!", | |
| }, | |
| wantSentData: types.ModuleCdc.MustMarshalJSON(&transfertypes.FungibleTokenPacketData{ | |
| Denom: "adym", | |
| Memo: addDenomMetadataToPacketData("thanks for the sweater, grandma!", validDenomMetadata), | |
| }), | |
| }, { | |
| name: "error: denom metadata already in memo", | |
| fields: fields{ | |
| ICS4Wrapper: &mockICS4Wrapper{}, | |
| }, | |
| data: &transfertypes.FungibleTokenPacketData{ | |
| Denom: "adym", | |
| Memo: `{"transferinject":{}}`, | |
| }, | |
| wantSentData: []byte(""), | |
| wantErr: types.ErrMemoTransferInjectAlreadyExists, | |
| }, { | |
| name: "error: extract rollapp from channel", | |
| fields: fields{ | |
| ICS4Wrapper: &mockICS4Wrapper{}, | |
| rollappKeeper: &mockRollappKeeper{ | |
| err: errortypes.ErrInvalidRequest, | |
| }, | |
| }, | |
| data: &transfertypes.FungibleTokenPacketData{ | |
| Denom: "adym", | |
| }, | |
| wantSentData: []byte(""), | |
| wantErr: errortypes.ErrInvalidRequest, | |
| }, { | |
| name: "send unaltered: rollapp not found", | |
| fields: fields{ | |
| ICS4Wrapper: &mockICS4Wrapper{}, | |
| rollappKeeper: &mockRollappKeeper{}, | |
| }, | |
| data: &transfertypes.FungibleTokenPacketData{ | |
| Denom: "adym", | |
| Memo: "user memo", | |
| }, | |
| wantSentData: types.ModuleCdc.MustMarshalJSON(&transfertypes.FungibleTokenPacketData{ | |
| Denom: "adym", | |
| Memo: "user memo", | |
| }), | |
| }, { | |
| name: "send unaltered: denom metadata already in rollapp", | |
| fields: fields{ | |
| ICS4Wrapper: &mockICS4Wrapper{}, | |
| rollappKeeper: &mockRollappKeeper{ | |
| returnRollapp: &rollapptypes.Rollapp{ | |
| RegisteredDenoms: []string{"adym"}, | |
| }, | |
| }, | |
| }, | |
| data: &transfertypes.FungibleTokenPacketData{ | |
| Denom: "adym", | |
| }, | |
| wantSentData: types.ModuleCdc.MustMarshalJSON(&transfertypes.FungibleTokenPacketData{ | |
| Denom: "adym", | |
| }), | |
| }, { | |
| name: "error: get denom metadata", | |
| fields: fields{ | |
| ICS4Wrapper: &mockICS4Wrapper{}, | |
| rollappKeeper: &mockRollappKeeper{ | |
| returnRollapp: &rollapptypes.Rollapp{}, | |
| }, | |
| bankKeeper: mockBankKeeper{}, | |
| }, | |
| data: &transfertypes.FungibleTokenPacketData{ | |
| Denom: "adym", | |
| }, | |
| wantSentData: types.ModuleCdc.MustMarshalJSON(&transfertypes.FungibleTokenPacketData{ | |
| Denom: "adym", | |
| }), | |
| }, | |
| } | |
| for _, tt := range tests { | |
| t.Run(tt.name, func(t *testing.T) { | |
| m := transferinject.NewIBCSendMiddleware(tt.fields.ICS4Wrapper, tt.fields.rollappKeeper, tt.fields.bankKeeper) | |
| data := types.ModuleCdc.MustMarshalJSON(tt.data) | |
| _, err := m.SendPacket( | |
| sdk.Context{}, | |
| &capabilitytypes.Capability{}, | |
| "transfer", | |
| "channel-0", | |
| clienttypes.Height{}, | |
| 0, | |
| data, | |
| ) | |
| if tt.wantErr == nil { | |
| require.NoError(t, err) | |
| } else { | |
| require.ErrorIs(t, err, tt.wantErr) | |
| } | |
| require.Equal(t, string(tt.wantSentData), string(tt.fields.ICS4Wrapper.(*mockICS4Wrapper).sentData)) | |
| }) | |
| } | |
| } | |
| if tt.wantErr == nil { | |
| require.NoError(t, err) | |
| } else { | |
| require.True(t, errors.Is(err, tt.wantErr)) | |
| } |
Improve subtest names to clearly describe what each test is verifying.
- name: "send unaltered: rollapp not found"
+ name: "send unaltered: rollapp not found should pass without alteration"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 TestIBCMiddleware_SendPacket(t *testing.T) { | |
| type fields struct { | |
| ICS4Wrapper porttypes.ICS4Wrapper | |
| rollappKeeper types.RollappKeeper | |
| bankKeeper types.BankKeeper | |
| } | |
| tests := []struct { | |
| name string | |
| fields fields | |
| data *transfertypes.FungibleTokenPacketData | |
| wantSentData []byte | |
| wantErr error | |
| }{ | |
| { | |
| name: "success: added denom metadata to memo", | |
| fields: fields{ | |
| ICS4Wrapper: &mockICS4Wrapper{}, | |
| rollappKeeper: &mockRollappKeeper{ | |
| returnRollapp: &rollapptypes.Rollapp{}, | |
| }, | |
| bankKeeper: mockBankKeeper{ | |
| returnMetadata: validDenomMetadata, | |
| }, | |
| }, | |
| data: &transfertypes.FungibleTokenPacketData{ | |
| Denom: "adym", | |
| }, | |
| wantSentData: types.ModuleCdc.MustMarshalJSON(&transfertypes.FungibleTokenPacketData{ | |
| Denom: "adym", | |
| Memo: addDenomMetadataToPacketData("", validDenomMetadata), | |
| }), | |
| }, { | |
| name: "success: added denom metadata to non-empty user memo", | |
| fields: fields{ | |
| ICS4Wrapper: &mockICS4Wrapper{}, | |
| rollappKeeper: &mockRollappKeeper{ | |
| returnRollapp: &rollapptypes.Rollapp{}, | |
| }, | |
| bankKeeper: mockBankKeeper{ | |
| returnMetadata: validDenomMetadata, | |
| }, | |
| }, | |
| data: &transfertypes.FungibleTokenPacketData{ | |
| Denom: "adym", | |
| Memo: "thanks for the sweater, grandma!", | |
| }, | |
| wantSentData: types.ModuleCdc.MustMarshalJSON(&transfertypes.FungibleTokenPacketData{ | |
| Denom: "adym", | |
| Memo: addDenomMetadataToPacketData("thanks for the sweater, grandma!", validDenomMetadata), | |
| }), | |
| }, { | |
| name: "error: denom metadata already in memo", | |
| fields: fields{ | |
| ICS4Wrapper: &mockICS4Wrapper{}, | |
| }, | |
| data: &transfertypes.FungibleTokenPacketData{ | |
| Denom: "adym", | |
| Memo: `{"transferinject":{}}`, | |
| }, | |
| wantSentData: []byte(""), | |
| wantErr: types.ErrMemoTransferInjectAlreadyExists, | |
| }, { | |
| name: "error: extract rollapp from channel", | |
| fields: fields{ | |
| ICS4Wrapper: &mockICS4Wrapper{}, | |
| rollappKeeper: &mockRollappKeeper{ | |
| err: errortypes.ErrInvalidRequest, | |
| }, | |
| }, | |
| data: &transfertypes.FungibleTokenPacketData{ | |
| Denom: "adym", | |
| }, | |
| wantSentData: []byte(""), | |
| wantErr: errortypes.ErrInvalidRequest, | |
| }, { | |
| name: "send unaltered: rollapp not found", | |
| fields: fields{ | |
| ICS4Wrapper: &mockICS4Wrapper{}, | |
| rollappKeeper: &mockRollappKeeper{}, | |
| }, | |
| data: &transfertypes.FungibleTokenPacketData{ | |
| Denom: "adym", | |
| Memo: "user memo", | |
| }, | |
| wantSentData: types.ModuleCdc.MustMarshalJSON(&transfertypes.FungibleTokenPacketData{ | |
| Denom: "adym", | |
| Memo: "user memo", | |
| }), | |
| }, { | |
| name: "send unaltered: denom metadata already in rollapp", | |
| fields: fields{ | |
| ICS4Wrapper: &mockICS4Wrapper{}, | |
| rollappKeeper: &mockRollappKeeper{ | |
| returnRollapp: &rollapptypes.Rollapp{ | |
| RegisteredDenoms: []string{"adym"}, | |
| }, | |
| }, | |
| }, | |
| data: &transfertypes.FungibleTokenPacketData{ | |
| Denom: "adym", | |
| }, | |
| wantSentData: types.ModuleCdc.MustMarshalJSON(&transfertypes.FungibleTokenPacketData{ | |
| Denom: "adym", | |
| }), | |
| }, { | |
| name: "error: get denom metadata", | |
| fields: fields{ | |
| ICS4Wrapper: &mockICS4Wrapper{}, | |
| rollappKeeper: &mockRollappKeeper{ | |
| returnRollapp: &rollapptypes.Rollapp{}, | |
| }, | |
| bankKeeper: mockBankKeeper{}, | |
| }, | |
| data: &transfertypes.FungibleTokenPacketData{ | |
| Denom: "adym", | |
| }, | |
| wantSentData: types.ModuleCdc.MustMarshalJSON(&transfertypes.FungibleTokenPacketData{ | |
| Denom: "adym", | |
| }), | |
| }, | |
| } | |
| for _, tt := range tests { | |
| t.Run(tt.name, func(t *testing.T) { | |
| m := transferinject.NewIBCSendMiddleware(tt.fields.ICS4Wrapper, tt.fields.rollappKeeper, tt.fields.bankKeeper) | |
| data := types.ModuleCdc.MustMarshalJSON(tt.data) | |
| _, err := m.SendPacket( | |
| sdk.Context{}, | |
| &capabilitytypes.Capability{}, | |
| "transfer", | |
| "channel-0", | |
| clienttypes.Height{}, | |
| 0, | |
| data, | |
| ) | |
| if tt.wantErr == nil { | |
| require.NoError(t, err) | |
| } else { | |
| require.ErrorIs(t, err, tt.wantErr) | |
| } | |
| require.Equal(t, string(tt.wantSentData), string(tt.fields.ICS4Wrapper.(*mockICS4Wrapper).sentData)) | |
| }) | |
| } | |
| } | |
| func TestIBCMiddleware_SendPacket(t *testing.T) { | |
| type fields struct { | |
| ICS4Wrapper porttypes.ICS4Wrapper | |
| rollappKeeper types.RollappKeeper | |
| bankKeeper types.BankKeeper | |
| } | |
| tests := []struct { | |
| name string | |
| fields fields | |
| data *transfertypes.FungibleTokenPacketData | |
| wantSentData []byte | |
| wantErr error | |
| }{ | |
| { | |
| name: "success: added denom metadata to memo", | |
| fields: fields{ | |
| ICS4Wrapper: &mockICS4Wrapper{}, | |
| rollappKeeper: &mockRollappKeeper{ | |
| returnRollapp: &rollapptypes.Rollapp{}, | |
| }, | |
| bankKeeper: mockBankKeeper{ | |
| returnMetadata: validDenomMetadata, | |
| }, | |
| }, | |
| data: &transfertypes.FungibleTokenPacketData{ | |
| Denom: "adym", | |
| }, | |
| wantSentData: types.ModuleCdc.MustMarshalJSON(&transfertypes.FungibleTokenPacketData{ | |
| Denom: "adym", | |
| Memo: addDenomMetadataToPacketData("", validDenomMetadata), | |
| }), | |
| }, { | |
| name: "success: added denom metadata to non-empty user memo", | |
| fields: fields{ | |
| ICS4Wrapper: &mockICS4Wrapper{}, | |
| rollappKeeper: &mockRollappKeeper{ | |
| returnRollapp: &rollapptypes.Rollapp{}, | |
| }, | |
| bankKeeper: mockBankKeeper{ | |
| returnMetadata: validDenomMetadata, | |
| }, | |
| }, | |
| data: &transfertypes.FungibleTokenPacketData{ | |
| Denom: "adym", | |
| Memo: "thanks for the sweater, grandma!", | |
| }, | |
| wantSentData: types.ModuleCdc.MustMarshalJSON(&transfertypes.FungibleTokenPacketData{ | |
| Denom: "adym", | |
| Memo: addDenomMetadataToPacketData("thanks for the sweater, grandma!", validDenomMetadata), | |
| }), | |
| }, { | |
| name: "error: denom metadata already in memo", | |
| fields: fields{ | |
| ICS4Wrapper: &mockICS4Wrapper{}, | |
| }, | |
| data: &transfertypes.FungibleTokenPacketData{ | |
| Denom: "adym", | |
| Memo: `{"transferinject":{}}`, | |
| }, | |
| wantSentData: []byte(""), | |
| wantErr: types.ErrMemoTransferInjectAlreadyExists, | |
| }, { | |
| name: "error: extract rollapp from channel", | |
| fields: fields{ | |
| ICS4Wrapper: &mockICS4Wrapper{}, | |
| rollappKeeper: &mockRollappKeeper{ | |
| err: errortypes.ErrInvalidRequest, | |
| }, | |
| }, | |
| data: &transfertypes.FungibleTokenPacketData{ | |
| Denom: "adym", | |
| }, | |
| wantSentData: []byte(""), | |
| wantErr: errortypes.ErrInvalidRequest, | |
| }, { | |
| name: "send unaltered: rollapp not found should pass without alteration", | |
| fields: fields{ | |
| ICS4Wrapper: &mockICS4Wrapper{}, | |
| rollappKeeper: &mockRollappKeeper{}, | |
| }, | |
| data: &transfertypes.FungibleTokenPacketData{ | |
| Denom: "adym", | |
| Memo: "user memo", | |
| }, | |
| wantSentData: types.ModuleCdc.MustMarshalJSON(&transfertypes.FungibleTokenPacketData{ | |
| Denom: "adym", | |
| Memo: "user memo", | |
| }), | |
| }, { | |
| name: "send unaltered: denom metadata already in rollapp", | |
| fields: fields{ | |
| ICS4Wrapper: &mockICS4Wrapper{}, | |
| rollappKeeper: &mockRollappKeeper{ | |
| returnRollapp: &rollapptypes.Rollapp{ | |
| RegisteredDenoms: []string{"adym"}, | |
| }, | |
| }, | |
| }, | |
| data: &transfertypes.FungibleTokenPacketData{ | |
| Denom: "adym", | |
| }, | |
| wantSentData: types.ModuleCdc.MustMarshalJSON(&transfertypes.FungibleTokenPacketData{ | |
| Denom: "adym", | |
| }), | |
| }, { | |
| name: "error: get denom metadata", | |
| fields: fields{ | |
| ICS4Wrapper: &mockICS4Wrapper{}, | |
| rollappKeeper: &mockRollappKeeper{ | |
| returnRollapp: &rollapptypes.Rollapp{}, | |
| }, | |
| bankKeeper: mockBankKeeper{}, | |
| }, | |
| data: &transfertypes.FungibleTokenPacketData{ | |
| Denom: "adym", | |
| }, | |
| wantSentData: types.ModuleCdc.MustMarshalJSON(&transfertypes.FungibleTokenPacketData{ | |
| Denom: "adym", | |
| }), | |
| }, | |
| } | |
| for _, tt := range tests { | |
| t.Run(tt.name, func(t *testing.T) { | |
| m := transferinject.NewIBCSendMiddleware(tt.fields.ICS4Wrapper, tt.fields.rollappKeeper, tt.fields.bankKeeper) | |
| data := types.ModuleCdc.MustMarshalJSON(tt.data) | |
| _, err := m.SendPacket( | |
| sdk.Context{}, | |
| &capabilitytypes.Capability{}, | |
| "transfer", | |
| "channel-0", | |
| clienttypes.Height{}, | |
| 0, | |
| data, | |
| ) | |
| if tt.wantErr == nil { | |
| require.NoError(t, err) | |
| } else { | |
| require.ErrorIs(t, err, tt.wantErr) | |
| } | |
| require.Equal(t, string(tt.wantSentData), string(tt.fields.ICS4Wrapper.(*mockICS4Wrapper).sentData)) | |
| }) | |
| } | |
| } |
| if Contains(rollapp.RegisteredDenoms, packet.Denom) { | ||
| return m.ICS4Wrapper.SendPacket(ctx, chanCap, destinationPort, destinationChannel, timeoutHeight, timeoutTimestamp, data) |
There was a problem hiding this comment.
here you use packet.Denom but in the ack code you use base
Can it be consistent?
There was a problem hiding this comment.
packet.Denom is from the packet, but in the ack handler base is from the injected denom metadata, but essentially it's the same
| @@ -41,12 +41,14 @@ message Rollapp { | |||
| repeated string permissionedAddresses = 5; | |||
| // tokenMetadata is a list of TokenMetadata that are registered on this rollapp | |||
| repeated TokenMetadata tokenMetadata = 6; | |||
| // registeredDenoms is a list of registered denoms on this rollapp | |||
There was a problem hiding this comment.
list of registered denoms
Is it a base or id or what?
|
Now that we only refer to the denom by a short string, what do we do if it get's updated in the bank? We need to ship the update or what? Is this specced out/documented? |
The base is the key, which doesn't get updated, but if anything else is updated, the base/key still references the same record in the bank |
|
So the rollapp wouldn't necessarily get the update then? |
| // At the first match, we assume that the rollapp already contains the metadata. | ||
| // It would be technically possible to have a race condition where the denom metadata is added to the rollapp | ||
| // from another packet before this packet is acknowledged. | ||
| if Contains(rollapp.RegisteredDenoms, packet.Denom) { |
There was a problem hiding this comment.
@zale144 don't u need to validate it's not a token sourced from the rollapp? otherwise u register the rollapp's token as it represented on the hub, makes it wierdly duplicate record on the rollapp
There was a problem hiding this comment.
the denom here is used as a key/reference, as it's found/represented on the hub (same one in the ack handler), just as an optimization, to avoid sending denom metadata in every packet. the point is to deduplicate on the hub.
There was a problem hiding this comment.
the entire middleware should work only for tokens other than tokens sourced on the rollapp.
| func ParsePacketMetadata(input string) (*TransferInject, error) { | ||
| bz := []byte(input) | ||
|
|
||
| var memo MemoData |
There was a problem hiding this comment.
@zale144 why sometimes u unmarshal into MemoData and sometimes into memoMap := make(map[string]any)
|
Could you please document the fact that the rollapp wont get updated if something is updated in the bank keeper but the base remains the same? |
|
Conflicts |
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
x/rollapp/types/rollapp.pb.gois excluded by!**/*.pb.go
Files selected for processing (3)
- proto/dymension/rollapp/rollapp.proto (1 hunks)
- x/transferinject/ibc_middleware.go (1 hunks)
- x/transferinject/ibc_middleware_test.go (1 hunks)
Files skipped from review due to trivial changes (1)
- proto/dymension/rollapp/rollapp.proto
Files skipped from review as they are similar to previous changes (1)
- x/transferinject/ibc_middleware.go
| rollappKeeper: &mockRollappKeeper{}, | ||
| }, | ||
| args: args{ | ||
| destinationPort: "port", | ||
| destinationChannel: "channel", | ||
| data: &transfertypes.FungibleTokenPacketData{ | ||
| Denom: "adym", | ||
| Memo: "user memo", | ||
| }, | ||
| }, | ||
| wantSentData: types.ModuleCdc.MustMarshalJSON(&transfertypes.FungibleTokenPacketData{ | ||
| Denom: "adym", | ||
| Memo: "user memo", | ||
| }), | ||
| }, { | ||
| name: "send unaltered: receiver chain is source", | ||
| fields: fields{ | ||
| ICS4Wrapper: &mockICS4Wrapper{}, | ||
| rollappKeeper: &mockRollappKeeper{ | ||
| returnRollapp: &rollapptypes.Rollapp{}, | ||
| }, | ||
| bankKeeper: mockBankKeeper{ | ||
| returnMetadata: validDenomMetadata, | ||
| }, | ||
| }, | ||
| args: args{ | ||
| destinationPort: "transfer", | ||
| destinationChannel: "channel-56", | ||
| data: &transfertypes.FungibleTokenPacketData{ | ||
| Denom: "transfer/channel-56/alex", | ||
| }, | ||
| }, | ||
| wantSentData: types.ModuleCdc.MustMarshalJSON(&transfertypes.FungibleTokenPacketData{ | ||
| Denom: "transfer/channel-56/alex", | ||
| }), | ||
| }, { | ||
| name: "send unaltered: denom metadata already in rollapp", | ||
| fields: fields{ | ||
| ICS4Wrapper: &mockICS4Wrapper{}, | ||
| rollappKeeper: &mockRollappKeeper{ | ||
| returnRollapp: &rollapptypes.Rollapp{ | ||
| RegisteredDenoms: []string{"adym"}, | ||
| }, | ||
| }, | ||
| }, | ||
| args: args{ | ||
| destinationPort: "port", | ||
| destinationChannel: "channel", | ||
| data: &transfertypes.FungibleTokenPacketData{ | ||
| Denom: "adym", | ||
| }, | ||
| }, | ||
| wantSentData: types.ModuleCdc.MustMarshalJSON(&transfertypes.FungibleTokenPacketData{ | ||
| Denom: "adym", | ||
| }), | ||
| }, { | ||
| name: "error: get denom metadata", | ||
| fields: fields{ | ||
| ICS4Wrapper: &mockICS4Wrapper{}, | ||
| rollappKeeper: &mockRollappKeeper{ | ||
| returnRollapp: &rollapptypes.Rollapp{}, | ||
| }, | ||
| bankKeeper: mockBankKeeper{}, | ||
| }, | ||
| args: args{ | ||
| destinationPort: "port", | ||
| destinationChannel: "channel", | ||
| data: &transfertypes.FungibleTokenPacketData{ | ||
| Denom: "adym", | ||
| }, | ||
| }, | ||
| wantSentData: types.ModuleCdc.MustMarshalJSON(&transfertypes.FungibleTokenPacketData{ | ||
| Denom: "adym", | ||
| }), | ||
| }, | ||
| } | ||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| m := transferinject.NewIBCSendMiddleware(tt.fields.ICS4Wrapper, tt.fields.rollappKeeper, tt.fields.bankKeeper) | ||
|
|
||
| data := types.ModuleCdc.MustMarshalJSON(tt.args.data) | ||
|
|
||
| _, err := m.SendPacket( | ||
| sdk.Context{}, | ||
| &capabilitytypes.Capability{}, | ||
| tt.args.destinationPort, | ||
| tt.args.destinationChannel, | ||
| clienttypes.Height{}, | ||
| 0, | ||
| data, | ||
| ) | ||
| if tt.wantErr == nil { | ||
| require.NoError(t, err) | ||
| } else { | ||
| require.ErrorIs(t, err, tt.wantErr) | ||
| } | ||
| require.Equal(t, string(tt.wantSentData), string(tt.fields.ICS4Wrapper.(*mockICS4Wrapper).sentData)) | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Refactor test cases to improve clarity and maintainability.
- Consider using table-driven tests with clearer names that describe the behavior being tested.
- Use
errors.Isfor error comparisons to improve consistency and reliability.
- require.ErrorIs(t, err, tt.wantErr)
+ require.True(t, errors.Is(err, tt.wantErr))- Enhance test names to reflect the expected outcome or behavior being tested.
- name: "send unaltered: rollapp not found"
+ name: "send unaltered: rollapp not found should pass without alteration"| IBCModule: mockIBCModule{}, | ||
| }, | ||
| args: args{ | ||
| packetData: &transfertypes.FungibleTokenPacketData{ | ||
| Denom: "adym", | ||
| Memo: "user memo", | ||
| }, | ||
| acknowledgement: okAck(), | ||
| }, | ||
| wantRollapp: nil, | ||
| }, { | ||
| name: "return early: no denom metadata in memo", | ||
| fields: fields{ | ||
| rollappKeeper: &mockRollappKeeper{}, | ||
| IBCModule: mockIBCModule{}, | ||
| }, | ||
| args: args{ | ||
| packetData: &transfertypes.FungibleTokenPacketData{ | ||
| Denom: "adym", | ||
| Memo: `{"transferinject":{}}`, | ||
| }, | ||
| acknowledgement: okAck(), | ||
| }, | ||
| wantRollapp: nil, | ||
| }, { | ||
| name: "error: extract rollapp from channel", | ||
| fields: fields{ | ||
| IBCModule: mockIBCModule{}, | ||
| rollappKeeper: &mockRollappKeeper{ | ||
| err: errortypes.ErrInvalidRequest, | ||
| }, | ||
| }, | ||
| args: args{ | ||
| packetData: &transfertypes.FungibleTokenPacketData{ | ||
| Denom: "adym", | ||
| Memo: addDenomMetadataToPacketData("", validDenomMetadata), | ||
| }, | ||
| acknowledgement: okAck(), | ||
| }, | ||
| wantRollapp: nil, | ||
| wantErr: errortypes.ErrInvalidRequest, | ||
| }, { | ||
| name: "error: rollapp not found", | ||
| fields: fields{ | ||
| IBCModule: mockIBCModule{}, | ||
| rollappKeeper: &mockRollappKeeper{}, | ||
| }, | ||
| args: args{ | ||
| packetData: &transfertypes.FungibleTokenPacketData{ | ||
| Denom: "adym", | ||
| Memo: addDenomMetadataToPacketData("", validDenomMetadata), | ||
| }, | ||
| acknowledgement: okAck(), | ||
| }, | ||
| wantRollapp: nil, | ||
| wantErr: errortypes.ErrNotFound, | ||
| }, { | ||
| name: "return early: rollapp already has token metadata", | ||
| fields: fields{ | ||
| IBCModule: mockIBCModule{}, | ||
| rollappKeeper: &mockRollappKeeper{ | ||
| returnRollapp: &rollapptypes.Rollapp{ | ||
| RegisteredDenoms: []string{validDenomMetadata.Base}, | ||
| }, | ||
| }, | ||
| }, | ||
| args: args{ | ||
| packetData: &transfertypes.FungibleTokenPacketData{ | ||
| Denom: "adym", | ||
| Memo: addDenomMetadataToPacketData("", validDenomMetadata), | ||
| }, | ||
| acknowledgement: okAck(), | ||
| }, | ||
| wantRollapp: &rollapptypes.Rollapp{ | ||
| RegisteredDenoms: []string{validDenomMetadata.Base}, | ||
| }, | ||
| }, | ||
| } | ||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| m := transferinject.NewIBCAckMiddleware(tt.fields.IBCModule, tt.fields.rollappKeeper) | ||
|
|
||
| packet := channeltypes.Packet{} | ||
|
|
||
| if tt.args.packetData != nil { | ||
| packet.Data = types.ModuleCdc.MustMarshalJSON(tt.args.packetData) | ||
| } | ||
|
|
||
| err := m.OnAcknowledgementPacket(sdk.Context{}, packet, tt.args.acknowledgement, sdk.AccAddress{}) | ||
|
|
||
| if tt.wantErr == nil { | ||
| require.NoError(t, err) | ||
| } else { | ||
| require.ErrorIs(t, err, tt.wantErr) | ||
| } | ||
|
|
||
| require.Equal(t, tt.wantRollapp, tt.fields.rollappKeeper.(*mockRollappKeeper).returnRollapp) | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Ensure consistent error handling and improve test case names in TestIBCMiddleware_OnAcknowledgementPacket.
- Use
errors.Isfor error comparisons to ensure consistency across tests. - Improve test names to clearly describe the expected behavior or outcome.
- name: "return early: error acknowledgement"
+ name: "return early: error acknowledgement should handle errors gracefully"Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (1)
x/bridging_fee/ibc_middleware.go (1)
Line range hint
31-64: RefactorBridgingFeeMiddlewareto improve error handling and logging.The implementation of
OnRecvPacketcould be improved by handling potential errors more gracefully and providing more detailed logging for debugging purposes. Consider adding more specific error messages and using structured logging to capture important packet and transaction details.- logger.Error("Failed to extract rollapp id from packet", "err", err) + logger.Error("Failed to extract rollapp id from packet", "error", err, "packet_data", packet.Data)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- app/app.go (8 hunks)
- ibctesting/eibc_test.go (4 hunks)
- x/bridging_fee/ibc_middleware.go (3 hunks)
- x/delayedack/eibc.go (4 hunks)
- x/delayedack/keeper/fraud_test.go (2 hunks)
- x/eibc/genesis_test.go (1 hunks)
Files skipped from review due to trivial changes (1)
- x/eibc/genesis_test.go
Additional comments not posted (5)
x/delayedack/keeper/fraud_test.go (1)
13-14: Ensure proper initialization and usage ofrollappKeeperin test setups.The addition of
rollappKeeperin the test setup is consistent with the changes in the middleware to accommodate the new functionality related to handling rollapp packets. This change ensures that the tests reflect the new architecture and are capable of testing the new logic effectively.Also applies to: 50-51
app/app.go (4)
623-634: Ensure correct integration ofNewIBCSendMiddlewareinTransferKeeper.The integration of
NewIBCSendMiddlewarewithTransferKeeperappears correctly implemented. This change aligns with the PR's objective to enhance IBC transfer capabilities. However, ensure that the middleware is thoroughly tested, especially since it interacts with multiple components likeIBCKeeper,RollappKeeper, andBankKeeper.
757-758: Review the integration ofNewIBCAckMiddlewarein the IBC middleware stack.The integration of
NewIBCAckMiddlewarewithin the IBC middleware stack is correctly implemented. This middleware is crucial for handling acknowledgments in the IBC transfer process, especially with the new functionality related to denom metadata migration. Ensure comprehensive testing of this middleware to verify its interaction withDelayedAckMiddlewareandRollappKeeper.
636-636: Correct method name toSetTransferKeeper.- app.RollappKeeper.SetTransferKeeper(app.TransferKeeper) + app.RollappKeeper.SetTransferKeeper(app.TransferKeeper)The method name
SetTransferKeeperis correctly used here, aligning with the changes noted in the AI-generated summary. This ensures that theRollappKeeperis properly configured with theTransferKeeper.Likely invalid or redundant comment.
770-770: Clarify the role ofdelayedAckMiddlewarein theSetHooksmethod.The use of
delayedAckMiddlewarein theSetHooksmethod ofRollappKeeperneeds clarification. It's important to understand how this middleware interacts with other components and its impact on the system, especially in the context of IBC transfers and denom metadata handling.Verification successful
The role of
delayedAckMiddlewarein theSetHooksmethod is now clearer based on the gathered information:
delayedAckMiddlewareis initialized usingdelayedackmodule.NewIBCMiddleware, which creates anIBCMiddlewareinstance with the providedkeeper,rollappKeeper, and underlying application.- It is then used in
transferinject.NewIBCAckMiddleware, which creates anIBCAckMiddlewarethat intercepts acknowledged incoming IBC packets and adds token metadata to the local rollapp record.This middleware is part of the IBC (Inter-Blockchain Communication) stack and is responsible for handling delayed acknowledgments and ensuring that token metadata is correctly registered on the rollapp.
app/app.go:
delayedAckMiddlewareis initialized and used in the IBC middleware stack.- It is passed to the
SetHooksmethod, indicating its role in handling IBC packet acknowledgments and metadata registration.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the role and integration of `delayedAckMiddleware` in the `SetHooks` method. # Test: Search for the usage of `delayedAckMiddleware` in the codebase. Expect: Clear documentation or comments explaining its role. rg --type go $'delayedAckMiddleware'Length of output: 300
Script:
#!/bin/bash # Description: Gather more information about `delayedAckMiddleware` by examining its initialization and usage. # Search for the definition of `NewIBCMiddleware` in the `delayedackmodule` package. rg --type go --pattern 'func NewIBCMiddleware' --context 5 # Search for the definition of `NewIBCAckMiddleware` in the `transferinject` package. rg --type go --pattern 'func NewIBCAckMiddleware' --context 5Length of output: 948
Script:
#!/bin/bash # Description: Gather more information about `delayedAckMiddleware` by examining its initialization and usage. # Search for the definition of `NewIBCMiddleware` in the `delayedackmodule` package. rg --type go 'func NewIBCMiddleware' --context 5 # Search for the definition of `NewIBCAckMiddleware` in the `transferinject` package. rg --type go 'func NewIBCAckMiddleware' --context 5Length of output: 2469
| feeMultiplier = im.TimeoutFee(ctx) | ||
| case commontypes.RollappPacket_ON_ACK: | ||
| feeMultiplier = im.keeper.ErrAckFee(ctx) | ||
| feeMultiplier = im.ErrAckFee(ctx) |
There was a problem hiding this comment.
Review and optimize the fee calculation logic in eIBCDemandOrderHandler.
The fee calculation logic could be optimized by consolidating the repeated code and ensuring that the fee calculations are accurate and efficient. Additionally, consider adding more comprehensive unit tests to cover all possible scenarios and edge cases related to fee calculations.
- feeMultiplier = im.TimeoutFee(ctx)
+ feeMultiplier = im.calculateFeeMultiplier(ctx, t)Also applies to: 67-67, 89-89, 129-129
Committable suggestion was skipped due to low confidence.
| ConvertToApp(suite.hubChain).BankKeeper.SetDenomMetaData(suite.hubChain.GetContext(), banktypes.Metadata{ | ||
| Base: sdk.DefaultBondDenom, | ||
| }) |
There was a problem hiding this comment.
Consider using a helper function for setting metadata.
To improve code readability and reusability, consider encapsulating the metadata setting logic into a helper function within the test suite. This will make the SetupTest method cleaner and more maintainable.
- ConvertToApp(suite.hubChain).BankKeeper.SetDenomMetaData(suite.hubChain.GetContext(), banktypes.Metadata{
- Base: sdk.DefaultBondDenom,
- })
+ suite.setDenomMetaData(sdk.DefaultBondDenom)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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ConvertToApp(suite.hubChain).BankKeeper.SetDenomMetaData(suite.hubChain.GetContext(), banktypes.Metadata{ | |
| Base: sdk.DefaultBondDenom, | |
| }) | |
| suite.setDenomMetaData(sdk.DefaultBondDenom) |
ibctesting/eibc_test.go
Outdated
| @@ -301,7 +306,7 @@ | |||
| rollappPacket, err := delayedAckKeeper.GetRollappPacket(suite.hubChain.GetContext(), lastDemandOrder.TrackingPacketKey) | |||
| suite.Require().NoError(err) | |||
| var data transfertypes.FungibleTokenPacketData | |||
| err = transfertypes.ModuleCdc.UnmarshalJSON(rollappPacket.Packet.GetData(), &data) | |||
| err = eibctypes.ModuleCdc.UnmarshalJSON(rollappPacket.Packet.GetData(), &data) | |||
There was a problem hiding this comment.
Use MustUnmarshalJSON for safer deserialization.
Using MustUnmarshalJSON instead of UnmarshalJSON can provide a safer way to handle JSON deserialization, as it panics if an error occurs, ensuring that tests fail fast if there is an issue with the data format.
- err = eibctypes.ModuleCdc.UnmarshalJSON(rollappPacket.Packet.GetData(), &data)
+ eibctypes.ModuleCdc.MustUnmarshalJSON(rollappPacket.Packet.GetData(), &data)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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| err = eibctypes.ModuleCdc.UnmarshalJSON(rollappPacket.Packet.GetData(), &data) | |
| eibctypes.ModuleCdc.MustUnmarshalJSON(rollappPacket.Packet.GetData(), &data) |
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (1)
x/bridging_fee/ibc_middleware.go (1)
72-72: Consider revising the error message to avoid phrases like "Failed to" as per PR comments.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- app/app.go (9 hunks)
- x/bridging_fee/ibc_middleware.go (3 hunks)
Additional comments not posted (5)
x/bridging_fee/ibc_middleware.go (3)
38-50: Constructor logic is clear and correctly initializes all properties ofBridgingFeeMiddleware.
49-49: MethodGetFeeRecipientcorrectly retrieves the fee recipient address.
72-72: Verify the logic for fee calculation and ensure it aligns with business requirements.app/app.go (2)
636-636: ### SetTransferKeeperinRollappKeeper
This line is crucial as it ensures that theRollappKeeperhas a reference to theTransferKeeper. This setup is necessary for theRollappKeeperto interact with the IBC functionalities.
- Correctness: Confirm that the
TransferKeeperis correctly set in theRollappKeeper.- Maintainability: This change helps in maintaining clear separation and interaction between different modules.
This change is well-integrated and follows good software engineering practices by ensuring that different modules can interact through well-defined interfaces.
777-777: ### Use ofdelayedAckMiddlewareinRollappKeeper
This line indicates the use ofdelayedAckMiddlewarein theRollappKeeper. It's important to ensure that this middleware is compatible and does not introduce issues.
- Compatibility: Check the compatibility of
delayedAckMiddlewarewithRollappKeeper.- Correctness: Ensure that the middleware does not alter the expected behavior of
RollappKeeper.The integration appears to be done correctly, but it would be prudent to monitor this closely during testing to ensure that there are no unexpected behaviors.
| transferStack = bridging_fee.NewIBCMiddleware( | ||
| transferMiddleware, | ||
| app.IBCKeeper.ChannelKeeper, | ||
| app.DelayedAckKeeper, | ||
| app.RollappKeeper, | ||
| app.TransferKeeper, | ||
| app.AccountKeeper.GetModuleAddress(txfeestypes.ModuleName), | ||
| ) |
There was a problem hiding this comment.
Introduction of bridging_fee.NewIBCMiddleware
The introduction of a new IBC middleware for handling bridging fees is a significant addition. This middleware is expected to handle the fees associated with IBC transfers.
- Correctness: Ensure that the middleware correctly calculates and applies bridging fees.
- Security: Review the fee handling to ensure that there are no potential security vulnerabilities such as integer overflows or underflows.
- Performance: Analyze the impact of this middleware on the performance of IBC transfers.
It is recommended to perform a security audit on the new bridging fee middleware to ensure that it handles fees securely and correctly.
| delayedAckMiddleware := delayedackmodule.NewIBCMiddleware(transferStack, app.DelayedAckKeeper, app.RollappKeeper) | ||
| transferStack = transferinject.NewIBCAckMiddleware(delayedAckMiddleware, app.RollappKeeper) |
There was a problem hiding this comment.
Integration of NewIBCAckMiddleware
The integration of NewIBCAckMiddleware is aimed at handling acknowledgments in IBC transfers. This is a critical part of ensuring that transfers are completed successfully.
- Correctness: Verify that the middleware correctly handles all cases of acknowledgments.
- Error Handling: Ensure that all error scenarios are handled properly.
- Testing: Adding unit tests for this middleware would help in ensuring its functionality and robustness.
Consider adding unit tests for the NewIBCAckMiddleware to ensure its correct functionality and robust handling of different scenarios.
|
Is this ready to be merged @zale144 ? |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- CHANGELOG.md (2 hunks)
- ibctesting/eibc_test.go (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- CHANGELOG.md
- ibctesting/eibc_test.go
Description
Closes #907
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow-up issues.
PR review checkboxes:
I have...
Unreleasedsection inCHANGELOG.mdgodoccommentsSDK Checklist
maptime.Now()sendCoinand notSendCoinsFull security checklist here
----;
For Reviewer:
---;
After reviewer approval:
Summary by CodeRabbit
New Features
rollappp.Bug Fixes
Refactor
Tests