feat: auth, authz, bank e2e tests for fiattokenfactory #373
Conversation
WalkthroughThe changes in this pull request introduce multiple new test functions and modify existing tests within the Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
| bal, err := noble.GetBalance(ctx, alice.FormattedAddress(), "ustake") | ||
| require.NoError(t, err) | ||
| require.Equal(t, originalAmount, bal) | ||
|
|
There was a problem hiding this comment.
What's the expected behaviour for the fees here? Are fees still charged (and so alice's USDC balance drops), or is the transaction dropped before fees are charged so the balance remains? Is this worth adding an assertion on?
There was a problem hiding this comment.
Good call. The fee is not taken.
Added a check in: 93925c1
| bal, err = noble.GetBalance(ctx, alice.FormattedAddress(), "ustake") | ||
| require.NoError(t, err) | ||
| require.Equal(t, originalAmount, bal) | ||
|
|
There was a problem hiding this comment.
same question about checking the usdc balance
e2e/fiat_tf_test.go
Outdated
|
|
||
| blacklistAccount(t, ctx, val, nw.fiatTfRoles.Blacklister, granter) | ||
|
|
||
| preSendBal := bal |
There was a problem hiding this comment.
Given that balance stays zero for this entire test (all requests fail), do we need this preSendBal variable? Can we always check that it is zero instead? Or is there a reason you prefer to do the checks in this way?
There was a problem hiding this comment.
Yes, good call. Simplified:
133af8f (#373)
| require.NoError(t, err) | ||
| // bal should not change | ||
| require.Equal(t, preSendBal, bal) | ||
| } |
There was a problem hiding this comment.
Is there any value in adding a happy path test here (no accounts blacklisted, chain not paused)?
There was a problem hiding this comment.
I think so, mainly to validate the test. Added:
b345e59 (#373)
Co-authored-by: Dan Kanefsky <dan@noble.xyz>
johnletey
force-pushed
the
dan/fiat-tf-send
branch
from
b345e59 to
c91009e
Compare
johnletey
changed the title
There was a problem hiding this comment.
LGTM! Passing on Cosmos SDK v0.50 ⚡
- ✅ Green CI for
TestFiatTFAuth(link) - ✅ Green CI for
TestFiatTFAuthzGrant(link) - ✅ Green CI for
TestFiatTFAuthzSend(link) - ✅ Green CI for
TestFiatTFBankSend(link)
NOTE: An expected error message had to be adjusted slightly due to the most recent code changes to the FiatTokenFactory send restriction (fc8a898). This is due to the error message incorrectly using the toAddr instead of the grantee here. cc @circle-smartin
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
e2e/fiat_tf_test.go (2)
1795-1810: Verify Balance After Failed Transaction Due to Paused ChainIn
TestFiatTFBankSend, when the chain is paused, and a send operation fails (lines 1795-1810), consider adding an assertion to verify that the sender's balance remains unchanged to ensure fees are not deducted.Add the following checks after the failed send attempt:
aliceBal, err := noble.GetBalance(ctx, alice.FormattedAddress(), "uusdc") require.NoError(t, err, "error getting balance") require.Equal(t, math.NewInt(mintAmount), aliceBal, "Alice's balance should remain unchanged when the transaction fails")This confirms that no funds are deducted from Alice's account when the transaction is rejected due to the paused chain.
1811-1824: Consistent Unblacklisting After TestsIn lines 1823-1824, after unblacklisting Alice, ensure similar cleanup is done for any other accounts blacklisted during the test to prevent side effects on subsequent tests.
Review the test to confirm all blacklisted accounts are unblacklisted at the end.
📜 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 (2)
e2e/fiat_tf_test.go (2)
1627-1652:⚠️ Potential issueClarify Expected Behavior When Granting Authz to Blacklisted Accounts
In the
TestFiatTFAuthzGrantfunction, when granting authorization to a blacklisted grantee or from a blacklisted granter, the test expects success:
- Lines 1627-1638: Granting authz to a blacklisted grantee (
grantee2) succeeds.- Lines 1641-1652: Granting authz from a blacklisted granter (
granter3) succeeds.Typically, blacklisted accounts should be restricted from performing such operations. Please verify if this behavior aligns with the intended security policies.
If blacklisted accounts should not be able to grant or receive authorizations, consider updating the implementation to prevent this and adjusting the test expectations accordingly. Otherwise, if this is acceptable, adding comments to clarify the rationale would be helpful.
1717-1718: Ensure Consistent Error Handling for Blacklisted GranterWhen the granter is blacklisted (line 1715), the error message correctly references the granter's address:
require.ErrorContains(t, err, fmt.Sprintf("an address (%s) is blacklisted and can not send tokens: unauthorized", granter.FormattedAddress()))However, verify that the application's error handling consistently identifies and reports blacklisted entities across different scenarios.
Run the following script to search for error messages related to blacklisted accounts to ensure consistency:
This will help identify any discrepancies in error messages related to blacklisted accounts.
✅ Verification successful
Error Handling for Blacklisted Granter is Consistent
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistent error messages for blacklisted accounts. # Search for error messages involving blacklisted accounts. rg --type go 'is blacklisted and can not' -A 2Length of output: 1698
| // ACTION: Execute an authz SEND using a TF token from a grantee who is blacklisted | ||
| // EXPECTED: Request fails; Even though grantee is acting on behalf of the granter, | ||
| // the granter still cannot execute `send` due to being blacklisted | ||
| // Status: | ||
| // Granter1 has authorized Grantee1 to send 100usdc from their wallet | ||
|
|
||
| e2e.BlacklistAccount(t, ctx, val, nw.FiatTfRoles.Blacklister, grantee) | ||
|
|
||
| _, err = val.AuthzExec(ctx, grantee, nestedCmd) | ||
| require.ErrorContains(t, err, fmt.Sprintf("an address (%s) is blacklisted and can not authorize tokens: unauthorized", receiver.FormattedAddress())) |
There was a problem hiding this comment.
Correct Error Message Check for Blacklisted Grantee
In TestFiatTFAuthzSend, when the grantee is blacklisted (line 1699), the error check references the receiver's address instead of the grantee's:
require.ErrorContains(t, err, fmt.Sprintf("an address (%s) is blacklisted and can not authorize tokens: unauthorized", receiver.FormattedAddress()))Since the grantee (grantee) is blacklisted, the error message should reference the grantee's address.
Apply this diff to correct the error message check:
- require.ErrorContains(t, err, fmt.Sprintf("an address (%s) is blacklisted and can not authorize tokens: unauthorized", receiver.FormattedAddress()))
+ require.ErrorContains(t, err, fmt.Sprintf("an address (%s) is blacklisted and can not authorize tokens: unauthorized", grantee.FormattedAddress()))This ensures that the test accurately validates the error message corresponding to the blacklisted grantee.
|
|
||
| // ACTION: Grant an authz SEND using a TF token from a granter who is blacklisted | ||
| // EXPECTED: Success; | ||
|
|
||
| w = interchaintest.GetAndFundTestUsers(t, ctx, "default", math.OneInt(), noble, noble) | ||
| granter3 := w[0] | ||
| grantee3 := w[1] | ||
|
|
||
| e2e.BlacklistAccount(t, ctx, val, nw.FiatTfRoles.Blacklister, granter3) | ||
|
|
||
| res, err = val.AuthzGrant(ctx, granter3, grantee3.FormattedAddress(), "send", "--spend-limit=100uusdc") | ||
| require.NoError(t, err) | ||
| require.Zero(t, res.Code) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider Validating Authorization Grants
After granting authorization (lines 1636-1652), it's good practice to verify that the authorization has been correctly recorded.
Add assertions to confirm that the grantee has the expected authorization:
res, err := val.AuthzListGrants(ctx, granter2, grantee2.FormattedAddress())
require.NoError(t, err)
require.Contains(t, res.Grants, /* expected grant details */)This ensures that the authorization grant is not only successful but also properly registered in the system.
| e2e.PauseFiatTF(t, ctx, val, nw.FiatTfRoles.Pauser) | ||
|
|
||
| _, err = val.AuthzExec(ctx, grantee, nestedCmd) | ||
| require.ErrorContains(t, err, "the chain is paused") | ||
|
|
||
| bal, err = noble.GetBalance(ctx, receiver.FormattedAddress(), "uusdc") | ||
| require.NoError(t, err) | ||
| require.True(t, bal.IsZero()) | ||
|
|
||
| e2e.UnpauseFiatTF(t, ctx, val, nw.FiatTfRoles.Pauser) | ||
|
|
There was a problem hiding this comment.
Ensure Proper Cleanup After Paused State
In TestFiatTFAuthzSend, after testing the paused state (lines 1747-1757), ensure that the TF is unpaused before proceeding to other tests to avoid side effects.
Add a call to e2e.UnpauseFiatTF if not already done, or confirm that the TF is unpaused at the end of the test.
// Ensure TF is unpaused after test
e2e.UnpauseFiatTF(t, ctx, val, nw.FiatTfRoles.Pauser)
Includes:
TestFiatTFAuthTestFiatTFAuthzGrantTestFiatTFAuthzSendTestFiatTFBankSend