feat: ibc e2e tests for fiattokenfactory #374
Conversation
WalkthroughThe changes in this pull request introduce new test cases in the Changes
Possibly related PRs
Suggested reviewers
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Co-authored-by: Dan Kanefsky <[email protected]>
johnletey
force-pushed
the
dan/fiat-tf-ibc
branch
from
5df6693
to
b6599b2
Compare
johnletey
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
e2e/fiat_tf_test.go (2)
1994-2000: Avoid reassigningamountToSendwith different valuesIn
TestFiatTFIBCIn, the variableamountToSendis assigned withmintAmountand later reassigned withmath.OneInt(). This can lead to confusion. Use separate variables or reset the value appropriately to maintain clarity.
2040-2041: Assert specific error details in IBC acknowledgementWhen checking the IBC acknowledgement for errors, consider asserting specific error codes or messages to make the test more precise and informative. This can help in quickly identifying the cause of failures.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- e2e/fiat_tf_test.go (2 hunks)
🧰 Additional context used
🔇 Additional comments (1)
e2e/fiat_tf_test.go (1)
2080-2098: Verify error handling when recipient is blacklisted during IBC transferEnsure that the test verifies the correct handling of errors when the recipient (
nobleWallet) is blacklisted during an IBC transfer. Confirm that the funds are not received and appropriate error acknowledgements are returned.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (4)
e2e/fiat_tf_test.go (4)
1852-2122: Ensure proper cleanup of test environmentsThe test functions do not explicitly clean up or teardown the test environments after execution. While the testing framework might handle some of this, it's good practice to ensure that all resources are properly released, especially when dealing with network or file system resources.
Consider adding defer statements to clean up resources, such as:
defer func() { if err := nw.Cleanup(); err != nil { t.Logf("Error during cleanup: %v", err) } }()This helps prevent resource leaks and makes the tests more robust.
1852-2122: Add comments to improve readabilityThe complex logic within the test functions can benefit from additional inline comments to explain the purpose of various steps, especially around error handling and IBC transfer processes.
For example, before the block handling the paused chain:
// Attempt IBC transfer while the token factory is paused. Expect failure.Adding such comments enhances maintainability and helps other developers understand the test flow.
13-17: Remove unused importsThe import
"github.com/strangelove-ventures/interchaintest/v8/testutil"is not used in the file. Unused imports can clutter the code and should be removed.Remove the unused import:
- "github.com/strangelove-ventures/interchaintest/v8/testutil"
14-14: Reorder imports for better organizationIt's a good practice to group imports into standard library packages, third-party packages, and local packages, separated by blank lines.
Reorder the imports as follows:
import ( "context" "encoding/json" "fmt" "testing" "cosmossdk.io/math" sdk "github.com/cosmos/cosmos-sdk/types" fiattokenfactorytypes "github.com/circlefin/noble-fiattokenfactory/x/fiattokenfactory/types" "github.com/cosmos/cosmos-sdk/crypto/keyring" transfertypes "github.com/cosmos/ibc-go/v8/modules/apps/transfer/types" "github.com/noble-assets/noble/e2e" "github.com/strangelove-ventures/interchaintest/v8" "github.com/strangelove-ventures/interchaintest/v8/ibc" "github.com/stretchr/testify/require" )This improves readability and follows Go's import conventions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
.github/workflows/e2e-tests.yamlis excluded by!**/*.yamle2e/go.modis excluded by!**/*.mod
📒 Files selected for processing (1)
- e2e/fiat_tf_test.go (2 hunks)
🔇 Additional comments (2)
e2e/fiat_tf_test.go (2)
1996-1999: Check for correct denomination in IBC transferIn the transfer object:
transfer := ibc.WalletAmount{ Address: nobleWallet.FormattedAddress(), Denom: dstIbcDenom, Amount: amountToSend, }Ensure that
dstIbcDenomis the correct denomination for the return IBC transfer from Gaia to Noble.Confirm that
dstIbcDenommatches the expected IBC denomination on Gaia for the token being transferred back to Noble.
1913-1925: Possible race condition when blacklisting accountsIn the test case where the noble wallet is blacklisted and then used in an IBC transfer, there might be a race condition if the unblacklisting happens before the transfer is attempted. Ensure that the blacklisted state is maintained throughout the test case.
Verify that the account remains blacklisted during the transfer attempt. You might want to add a check to confirm the blacklisted status:
Includes:
TestFiatTFIBCOutTestFiatTFIBCIn