feat: pause e2e tests for fiattokenfactory #369
Conversation
WalkthroughThe changes introduce new test functions in 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 (
|
| }, | ||
| } | ||
| require.Equal(t, expectedGetPauserResponse.Pauser, showPauserRes.Pauser) | ||
| } |
There was a problem hiding this comment.
Could we add the happy path case as well? (i.e. a normal request — nothing is paused or blacklisted)
There was a problem hiding this comment.
Added in: 9a77cc4 (#369)
| } | ||
| require.Equal(t, expectedPaused, showPausedRes) | ||
|
|
||
| // ACTION: Pause TF from a blacklisted Pauser account |
There was a problem hiding this comment.
Seems that assertions of the expected result are missing from this case and the one below?
There was a problem hiding this comment.
We do an assertion inside the pauseFiatTF function:
Lines 421 to 433 in 149a7f6
| // Paused: true | ||
|
|
||
| pauseFiatTF(t, ctx, val, nw.fiatTfRoles.Pauser) | ||
| } |
There was a problem hiding this comment.
Can we add a happy path test here? (precondition: valid pause request while nothing is paused or blacklisted)
There was a problem hiding this comment.
Added in: 9a77cc4 (#369)
e2e/fiat_tf_test.go
Outdated
|
|
||
| unblacklistAccount(t, ctx, val, nw.fiatTfRoles.Blacklister, nw.fiatTfRoles.Pauser) | ||
|
|
||
| // ACTION: Unpause TF while TF is already paused |
There was a problem hiding this comment.
I think you might have mixed up this case—we want the precondition to be that TF is unpaused, and unpausing it in this state results in the module remaining unpaused.
The EXPECTED comment implies you were intending on testing this, and the ACTION comment might have a typo? But the actual test implementation is testing unpausing of an already paused TF.
| } | ||
| require.Equal(t, expectedPaused, showPausedRes) | ||
|
|
||
| // ACTION: Unpause TF from a blacklisted Pauser account |
There was a problem hiding this comment.
Missing assertions of expected result for this test and the one below
There was a problem hiding this comment.
We do this assertion in the unpauseFiatTF function:
Lines 445 to 454 in 149a7f6
e2e/fiat_tf_test.go
Outdated
|
|
||
| unpauseFiatTF(t, ctx, val, nw.fiatTfRoles.Pauser) | ||
|
|
||
| unpauseFiatTF(t, ctx, val, nw.fiatTfRoles.Pauser) |
| unpauseFiatTF(t, ctx, val, nw.fiatTfRoles.Pauser) | ||
|
|
||
| unpauseFiatTF(t, ctx, val, nw.fiatTfRoles.Pauser) | ||
| } |
There was a problem hiding this comment.
Can we get a happy path test here as well? So unpausing a paused TF with an unblacklisted pauser.
There was a problem hiding this comment.
Added in: 9a77cc4 (#369)
Co-authored-by: Dan Kanefsky <dan@noble.xyz>
johnletey
force-pushed
the
dan/fiat-tf-pause
branch
from
9a77cc4 to
062f46c
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: 1
🧹 Outside diff range and nitpick comments (1)
e2e/utils.go (1)
Line range hint
664-726: Consider adopting a consistent approach to function visibility.The changes in this file have made some functions public (
ShowPauserandShowPaused) while leaving others private (showBlacklisterandshowBlacklisted). To improve code consistency and maintainability, consider adopting a uniform approach to function visibility in this utility file. If there are specific reasons for keeping certain functions private, it might be helpful to add comments explaining these decisions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- e2e/fiat_tf_test.go (1 hunks)
- e2e/utils.go (2 hunks)
🧰 Additional context used
🔇 Additional comments (6)
e2e/utils.go (4)
664-665: LGTM: Function renamed and made public.The
showPauserfunction has been renamed toShowPauser, making it public. This change increases the function's visibility, which is likely intentional for testing purposes. The implementation remains correct and follows good practices.Also applies to: 666-677
Line range hint
679-692: Verify: Inconsistent visibility forshowBlacklister.The
showBlacklisterfunction remains private, while similar functions likeShowPauserhave been made public. Is this intentional? If not, consider renaming it toShowBlacklisterfor consistency with the other utility functions.
Line range hint
694-708: Verify: Inconsistent visibility forshowBlacklisted.The
showBlacklistedfunction remains private, while similar functions likeShowPauserhave been made public. Is this intentional? If not, consider renaming it toShowBlacklistedfor consistency with the other utility functions.
713-714: LGTM: Function renamed and made public.The
showPausedfunction has been renamed toShowPaused, making it public. This change increases the function's visibility and is consistent with theShowPauserfunction. The implementation remains correct and follows good practices.Also applies to: 715-726
e2e/fiat_tf_test.go (2)
891-947: Tests Effectively Cover Pausing ScenariosThe
TestFiatTFPausefunction thoroughly tests the pausing functionality of the Token Factory (TF). It includes cases for:
- Successful pausing by the authorized pauser.
- Attempted pausing by an unauthorized account.
- Pausing by a blacklisted pauser account.
- Attempting to pause when the TF is already paused.
These tests ensure that the TF behaves correctly under various conditions.
949-1005: Comprehensive Testing of Unpausing FunctionalityThe
TestFiatTFUnpausefunction effectively verifies the unpausing behavior of the TF. It includes:
- Successful unpausing by the authorized pauser.
- Attempted unpausing by an unauthorized account.
- Unpausing by a blacklisted pauser account.
- Attempting to unpause when the TF is already unpaused.
The tests cover key scenarios to validate that the unpausing mechanism operates as expected.
| // EXPECTED: Success; pauser updated | ||
|
|
||
| w := interchaintest.GetAndFundTestUsers(t, ctx, "new-pauser-1", math.OneInt(), noble) | ||
| newPauser1 := w[0] | ||
|
|
||
| _, err := val.ExecTx(ctx, nw.FiatTfRoles.Owner.KeyName(), "fiat-tokenfactory", "update-pauser", newPauser1.FormattedAddress()) | ||
| require.NoError(t, err, "failed to broadcast update-pauser message") | ||
|
|
||
| showPauserRes, err := e2e.ShowPauser(ctx, val) | ||
| require.NoError(t, err, "failed to query show-pauser") | ||
| expectedGetPauserResponse := fiattokenfactorytypes.QueryGetPauserResponse{ | ||
| Pauser: fiattokenfactorytypes.Pauser{ | ||
| Address: newPauser1.FormattedAddress(), | ||
| }, | ||
| } | ||
| require.Equal(t, expectedGetPauserResponse.Pauser, showPauserRes.Pauser) | ||
|
|
||
| // ACTION: Update Pauser while TF is paused | ||
| // EXPECTED: Success; pauser updated | ||
| // Status: | ||
| // Pauser: newPauser1 | ||
|
|
||
| e2e.PauseFiatTF(t, ctx, val, newPauser1) | ||
|
|
||
| w = interchaintest.GetAndFundTestUsers(t, ctx, "new-pauser-2", math.OneInt(), noble) | ||
| newPauser2 := w[0] | ||
|
|
||
| _, err = val.ExecTx(ctx, nw.FiatTfRoles.Owner.KeyName(), "fiat-tokenfactory", "update-pauser", newPauser2.FormattedAddress()) | ||
| require.NoError(t, err, "failed to broadcast update-pauser message") | ||
|
|
||
| showPauserRes, err = e2e.ShowPauser(ctx, val) | ||
| require.NoError(t, err, "failed to query show-pauser") | ||
| expectedGetPauserResponse = fiattokenfactorytypes.QueryGetPauserResponse{ | ||
| Pauser: fiattokenfactorytypes.Pauser{ | ||
| Address: newPauser2.FormattedAddress(), | ||
| }, | ||
| } | ||
| require.Equal(t, expectedGetPauserResponse.Pauser, showPauserRes.Pauser) | ||
|
|
||
| e2e.UnpauseFiatTF(t, ctx, val, newPauser2) | ||
|
|
||
| // ACTION: Update Pauser from non owner account | ||
| // EXPECTED: Request fails; pauser not updated | ||
| // Status: | ||
| // Pauser: newPauser2 | ||
|
|
||
| w = interchaintest.GetAndFundTestUsers(t, ctx, "default", math.OneInt(), noble, noble) | ||
| newPauser3 := w[0] | ||
| alice := w[1] | ||
|
|
||
| _, err = val.ExecTx(ctx, alice.KeyName(), "fiat-tokenfactory", "update-pauser", newPauser3.FormattedAddress()) | ||
| require.ErrorContains(t, err, "you are not the owner: unauthorized") | ||
|
|
||
| showPauserRes, err = e2e.ShowPauser(ctx, val) | ||
| require.NoError(t, err, "failed to query show-pauser") | ||
| require.Equal(t, expectedGetPauserResponse.Pauser, showPauserRes.Pauser) | ||
|
|
||
| // ACTION: Update Pauser from blacklisted owner account | ||
| // EXPECTED: Success; pauser updated | ||
| // Status: | ||
| // Pauser: newPauser2 | ||
|
|
||
| e2e.BlacklistAccount(t, ctx, val, nw.FiatTfRoles.Blacklister, nw.FiatTfRoles.Owner) | ||
|
|
||
| _, err = val.ExecTx(ctx, nw.FiatTfRoles.Owner.KeyName(), "fiat-tokenfactory", "update-pauser", newPauser3.FormattedAddress()) | ||
| require.NoError(t, err, "failed to broadcast update-pauser message") | ||
|
|
||
| showPauserRes, err = e2e.ShowPauser(ctx, val) | ||
| require.NoError(t, err, "failed to query show-pauser") | ||
| expectedGetPauserResponse = fiattokenfactorytypes.QueryGetPauserResponse{ | ||
| Pauser: fiattokenfactorytypes.Pauser{ | ||
| Address: newPauser3.FormattedAddress(), | ||
| }, | ||
| } | ||
| require.Equal(t, expectedGetPauserResponse.Pauser, showPauserRes.Pauser) | ||
|
|
||
| e2e.UnblacklistAccount(t, ctx, val, nw.FiatTfRoles.Blacklister, nw.FiatTfRoles.Owner) | ||
|
|
||
| // ACTION: Update Pauser to blacklisted Pauser account | ||
| // EXPECTED: Success; pauser updated | ||
| // Status: | ||
| // Pauser: newPauser3 | ||
|
|
||
| w = interchaintest.GetAndFundTestUsers(t, ctx, "new-pauser-4", math.OneInt(), noble) | ||
| newPauser4 := w[0] | ||
|
|
||
| e2e.BlacklistAccount(t, ctx, val, nw.FiatTfRoles.Blacklister, newPauser4) | ||
|
|
||
| _, err = val.ExecTx(ctx, nw.FiatTfRoles.Owner.KeyName(), "fiat-tokenfactory", "update-pauser", newPauser4.FormattedAddress()) | ||
| require.NoError(t, err, "failed to broadcast update-pauser message") | ||
|
|
||
| showPauserRes, err = e2e.ShowPauser(ctx, val) | ||
| require.NoError(t, err, "failed to query show-pauser") | ||
| expectedGetPauserResponse = fiattokenfactorytypes.QueryGetPauserResponse{ | ||
| Pauser: fiattokenfactorytypes.Pauser{ | ||
| Address: newPauser4.FormattedAddress(), | ||
| }, | ||
| } | ||
| require.Equal(t, expectedGetPauserResponse.Pauser, showPauserRes.Pauser) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider Refactoring Test Setup to Reduce Code Duplication
The TestFiatTFUpdatePauser function contains multiple sections where new users are created and funded in a similar manner. Refactoring this repetitive code into a helper function can improve readability and maintainability.
You could create a helper function to handle user creation and funding:
func TestFiatTFUpdatePauser(t *testing.T) {
// ...
- w := interchaintest.GetAndFundTestUsers(t, ctx, "new-pauser-1", math.OneInt(), noble)
- newPauser1 := w[0]
+ newPauser1 := createAndFundUser(t, ctx, noble, "new-pauser-1")
// ...
- w = interchaintest.GetAndFundTestUsers(t, ctx, "new-pauser-2", math.OneInt(), noble)
- newPauser2 := w[0]
+ newPauser2 := createAndFundUser(t, ctx, noble, "new-pauser-2")
// ...
+func createAndFundUser(t *testing.T, ctx context.Context, chain *e2e.Chain, username string) *interchaintest.User {
+ users := interchaintest.GetAndFundTestUsers(t, ctx, username, math.OneInt(), chain)
+ return users[0]
+}Committable suggestion was skipped due to low confidence.
Includes:
TestFiatTFUpdatePauserTestFiatTFPauseTestFiatTFUnpause