feat: minter e2e tests for fiattokenfactory #368
Conversation
WalkthroughThe changes in this pull request enhance the testing suite for the fiat token factory by adding several 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 (
|
e2e/fiat_tf_test.go
Outdated
| // minterController1 -> minter4 | ||
| // minterController2 -> minter2 | ||
| // minterController3 -> minter3 | ||
| // minter1 dissociated from minter but has allowance |
e2e/fiat_tf_test.go
Outdated
| // minterController1 -> minter4 | ||
| // minterController2 -> minter2 | ||
| // minterController3 -> minter3 | ||
| // minter1 dissociated from minter but has allowance |
There was a problem hiding this comment.
nit: reusing existing states in the same test makes the test more brittle. i.e. it would be better to create dedicated accounts for each test-case instead of reusing them.
e2e/fiat_tf_test.go
Outdated
| // ACTION: Happy path: Configure minter | ||
| // EXPECTED: Success; Minter is configured with allowance | ||
|
|
||
| configureMinter(t, ctx, val, nw.fiatTfRoles.MinterController, nw.fiatTfRoles.Minter, 20) |
There was a problem hiding this comment.
missing: verifying the minter allowance was set properly.
There was a problem hiding this comment.
Verification is done inside the configureMinter function. See:
Lines 483 to 499 in 9143e5c
e2e/fiat_tf_test.go
Outdated
|
|
||
| blacklistAccount(t, ctx, val, nw.fiatTfRoles.Blacklister, minter1) | ||
|
|
||
| configureMinter(t, ctx, val, minterController1, minter1, 11) |
There was a problem hiding this comment.
missing, verify the minter was configured and the allowance is set properly.
There was a problem hiding this comment.
Co-authored-by: Dan Kanefsky <dan@noble.xyz>
johnletey
force-pushed
the
dan/fiat-tf-minter
branch
from
70e992c to
d8344e3
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: 0
🧹 Outside diff range and nitpick comments (6)
e2e/utils.go (4)
Line range hint
683-683: Consider renamingshowBlacklistertoShowBlacklisterFor consistency with the other exported functions in this file, consider renaming
showBlacklistertoShowBlacklister. This would make it an exported function and follow the same naming convention as the other utility functions in this file.
Line range hint
700-700: Consider renamingshowBlacklistedtoShowBlacklistedFor consistency with the other exported functions in this file, consider renaming
showBlacklistedtoShowBlacklisted. This would make it an exported function and follow the same naming convention as the other utility functions in this file.
Line range hint
718-718: Consider renamingshowPausedtoShowPausedFor consistency with the other exported functions in this file, consider renaming
showPausedtoShowPaused. This would make it an exported function and follow the same naming convention as the other utility functions in this file.
Line range hint
1-734: Overall improvements in naming conventions with a few remaining inconsistenciesThe changes in this file primarily focus on renaming functions to follow Go's naming conventions for exported functions. This improves the overall consistency and adherence to Go best practices. Most functions have been correctly renamed, such as
UnblacklistAccount,SetupMinterAndController,ConfigureMinter, etc.However, there are still three functions that haven't been renamed:
showBlacklistershowBlacklistedshowPausedFor complete consistency, consider renaming these functions to
ShowBlacklister,ShowBlacklisted, andShowPausedrespectively.No logic changes were made to any of the functions, which is appropriate for this type of refactoring.
e2e/fiat_tf_test.go (2)
593-594: Typo in comment: "Remove a a non existent Minter Controller"There's a duplication of the word "a" in the comment. It should be:
-// ACTION: Remove a a non existent Minter Controller +// ACTION: Remove a non-existent Minter Controller
681-682: Clarify the action comment for better understandingThe action comment is grammatically incorrect:
-// ACTION: Configure a minter is blacklisted from a blacklisted minter controller +// ACTION: Configure a blacklisted minter from a blacklisted minter controllerThis rephrasing improves clarity for future maintainers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- e2e/fiat_tf_test.go (2 hunks)
- e2e/utils.go (6 hunks)
🧰 Additional context used
🔇 Additional comments (11)
e2e/utils.go (6)
501-503: LGTM: Function renamed to follow Go conventionsThe function has been correctly renamed from
unblacklistAccounttoUnblacklistAccount, following Go's naming convention for exported functions. The implementation remains unchanged and correct.
553-554: LGTM: Function renamed to follow Go conventionsThe function has been correctly renamed from
setupMinterAndControllertoSetupMinterAndController, adhering to Go's naming convention for exported functions. The implementation remains unchanged and correct.
577-579: LGTM: Function renamed to follow Go conventionsThe function has been appropriately renamed from
configureMintertoConfigureMinter, following Go's naming convention for exported functions. The implementation remains unchanged and correct.
598-600: LGTM: Function renamed to follow Go conventionsThe function has been correctly renamed from
showMinterControllertoShowMinterController, adhering to Go's naming convention for exported functions. The implementation remains unchanged and correct.
615-617: LGTM: Function renamed to follow Go conventionsThe function has been appropriately renamed from
showMinterstoShowMinters, following Go's naming convention for exported functions. The implementation remains unchanged and correct.
648-649: LGTM: Function renamed to follow Go conventionsThe function has been correctly renamed from
showMasterMintertoShowMasterMinter, adhering to Go's naming convention for exported functions. The implementation remains unchanged and correct.e2e/fiat_tf_test.go (5)
178-291:TestFiatTFUpdateMasterMintertest cases are comprehensive and well-structuredThe test cases thoroughly cover scenarios for updating the Master Minter, including:
- Happy path updates.
- Updates while the token factory is paused.
- Unauthorized update attempts.
- Updates involving blacklisted accounts.
Assertions are appropriately used to verify expected outcomes, ensuring the functionality behaves as intended.
293-506:TestFiatTFConfigureMinterControllertests effectively validate minter controller configurationsThis test function comprehensively covers:
- Configuring minter controllers in normal conditions.
- Configurations while the token factory is paused.
- Unauthorized configuration attempts.
- Configurations involving blacklisted minter controllers and minters.
- Reassigning minter controllers to new minters.
- Assigning multiple controllers to a single minter.
The tests are well-implemented, and assertions confirm that the system state aligns with expectations after each action.
507-602:TestFiatTFRemoveMinterControllertests cover essential removal scenariosThe test cases include:
- Happy path removal of a minter controller.
- Removal while the token factory is paused.
- Unauthorized removal attempts.
- Removal involving blacklisted master minter and minter controller.
- Attempting to remove a non-existent minter controller.
Assertions are correctly used to validate that the minter controllers are removed or maintained as expected.
603-692:TestFiatTFConfigureMintertests are thorough and correctly implementedThe test function covers:
- Configuring a minter in normal conditions.
- Attempting to configure a minter while the token factory is paused (expecting failure).
- Unauthorized configuration attempts from incorrect minter controllers.
- Configuring a blacklisted minter from a blacklisted minter controller.
The tests effectively validate the expected behaviors, and the assertions confirm the system responds appropriately in each scenario.
694-775:TestFiatTFRemoveMintertests effectively cover minter removal scenariosThe test cases include:
- Happy path removal of a minter.
- Removal while the token factory is paused.
- Unauthorized removal attempts by a minter controller not associated with the minter.
- Removal involving blacklisted minter controller and minter.
Assertions are appropriately used to verify that minters are removed or retained as expected in each scenario.
Includes:
TestFiatTFUpdateMasterMinterTestFiatTFConfigureMinterControllerTestFiatTFRemoveMinterControllerTestFiatTFConfigureMinterTestFiatTFRemoveMinter