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 (
|
e2e/fiat_tf_test.go
Outdated
| // uusdc IBC denom on gaia | ||
| srcDenomTrace := transfertypes.ParseDenomTrace(transfertypes.GetPrefixedDenom("transfer", gaiaToNobleChannelID, denomMetadataUsdc.Base)) | ||
| dstIbcDenom := srcDenomTrace.IBCDenom() |
There was a problem hiding this comment.
If possible, these lines might fit in better with the setup (around line 44) rather than as part of the first test case since they appear to be shared across all the cases.
| gaiaWalletBal, err = gaia.GetBalance(ctx, gaiaWallet.FormattedAddress(), dstIbcDenom) | ||
| require.NoError(t, err) | ||
| require.Equal(t, transfer.Amount, gaiaWalletBal) | ||
| } |
There was a problem hiding this comment.
Any value in checking the noble wallet balance in any of the above cases? Or it's sufficient to just check the gaia balance?
There was a problem hiding this comment.
I think it is unnecessary as this is more testing core IBC functionality. However, I am happy to add the checks if you prefer.
There was a problem hiding this comment.
Ok, I think this is sufficient for me. Thanks!
| nobleWalletBal, err = noble.GetBalance(ctx, nobleWallet.FormattedAddress(), denomMetadataUsdc.Base) | ||
| require.NoError(t, err) | ||
| require.Equal(t, transfer.Amount, nobleWalletBal) | ||
| } |
There was a problem hiding this comment.
same question about checking the gaia balances in addition to the noble ones
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
| require.NoError(t, err) | ||
|
|
||
| ack, err := testutil.PollForAck(ctx, gaia, height, heightAfterFlush+5, tx.Packet) | ||
| require.NoError(t, err, "error polling for ack") |
There was a problem hiding this comment.
@boojamya We are running into the encoding issue we were experiencing with conformance tests 🤔
Maybe we can simply wait 5 blocks after flush, and that will be sufficient?
![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.
| func TestFiatTFIBCOut(t *testing.T) { | ||
| if testing.Short() { | ||
| t.Skip() | ||
| } | ||
| t.Parallel() | ||
|
|
||
| ctx := context.Background() | ||
|
|
||
| nw, gaia, _, r, ibcPathName, _, eRep, _, _ := e2e.NobleSpinUpIBC(t, ctx, true) | ||
| noble := nw.Chain | ||
| val := noble.Validators[0] | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor common test setup code into a helper function
The test functions TestFiatTFIBCOut and TestFiatTFIBCIn share similar setup code, including initializing contexts, spinning up networks, and creating wallets. Extracting these steps into a helper function can reduce code duplication and improve maintainability.
| nobleToGaiaChannelInfo, err := r.GetChannels(ctx, eRep, noble.Config().ChainID) | ||
| require.NoError(t, err) | ||
| nobleToGaiaChannelID := nobleToGaiaChannelInfo[0].ChannelID | ||
| // gaia -> noble channel info | ||
| gaiaToNobleChannelInfo, err := r.GetChannels(ctx, eRep, gaia.Config().ChainID) | ||
| require.NoError(t, err) | ||
| gaiaToNobleChannelID := gaiaToNobleChannelInfo[0].ChannelID | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Encapsulate channel retrieval logic into a utility function
The code for retrieving channel information between Noble and Gaia is duplicated. Consider creating a utility function to encapsulate the channel retrieval logic, which will enhance readability and reduce redundancy.
Includes:
TestFiatTFIBCOutTestFiatTFIBCIn