-
Notifications
You must be signed in to change notification settings - Fork 50
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: owner e2e tests for fiattokenfactory #367
Conversation
WalkthroughThe changes introduce a new file Changes
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 (
|
Co-authored-by: Dan Kanefsky <dan@noble.xyz>
e03d7f4
to
2933ae0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
e2e/utils.go (1)
Line range hint
1-733
: Consider reviewing other functions for potential exportingThe changes made to rename
BlacklistAccount
,PauseFiatTF
,UnpauseFiatTF
, andShowOwner
improve the code's adherence to Go's naming conventions. To maintain consistency, it might be worth reviewing other functions in this file (e.g.,unblacklistAccount
,showMinterController
,showMinters
, etc.) to determine if they should also be exported (renamed to start with an uppercase letter) if they are intended to be used outside this package.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- e2e/fiat_tf_test.go (1 hunks)
- e2e/utils.go (4 hunks)
🧰 Additional context used
🔇 Additional comments (10)
e2e/utils.go (4)
479-481
: LGTM: Function renamed to follow Go naming conventionsThe function
blacklistAccount
has been correctly renamed toBlacklistAccount
to follow Go's naming convention for exported functions. The change is consistent with the code's functionality, and the comment has been updated accordingly.Also applies to: 481-496
511-513
: LGTM: Function renamed to follow Go naming conventionsThe function
pauseFiatTF
has been correctly renamed toPauseFiatTF
to follow Go's naming convention for exported functions. The change is consistent with the code's functionality, and the comment has been updated accordingly.Also applies to: 513-531
532-534
: LGTM: Function renamed to follow Go naming conventionsThe function
unpauseFiatTF
has been correctly renamed toUnpauseFiatTF
to follow Go's naming convention for exported functions. The change is consistent with the code's functionality, and the comment has been updated accordingly.Also applies to: 534-552
632-633
: LGTM: Function renamed to follow Go naming conventionsThe function
showOwner
has been correctly renamed toShowOwner
to follow Go's naming convention for exported functions. The change is consistent with the code's functionality, and the comment has been updated accordingly.Also applies to: 633-645
e2e/fiat_tf_test.go (6)
58-61
: Ensure that setting a blacklisted account as the new owner is intentional.The test updates the owner to
newOwner1
, who has been blacklisted, and the operation succeeds. Assigning ownership to a blacklisted account may prevent the new owner from executing critical functions. Verify whether this behavior is desired and aligns with the system's security requirements.Use the following script to determine if the
UpdateOwner
function checks for blacklisted addresses when setting a new owner:#!/bin/bash # Description: Verify if the `UpdateOwner` action prohibits setting a blacklisted account as the owner. # Locate the `UpdateOwner` handler in the fiat token factory module. rg --type go 'func (k|c) .*UpdateOwner\(.*\) .* \{' -A 50 # Search for any validation that checks if the new owner is blacklisted. rg --type go 'IsBlacklisted\('
147-148
: Ensure the error message aligns with unauthorized access standards.When an unauthorized user attempts to accept ownership, the error expected is "you are not the pending owner: unauthorized". Confirm that this error message is consistent with standard unauthorized access errors in the codebase.
Run this script to verify the error handling:
#!/bin/bash # Description: Find the source of the unauthorized error message for `accept-owner`. # Search for the error message in the codebase. rg --type go '"you are not the pending owner: unauthorized"' # Check if it wraps the standard unauthorized error. rg --type go 'sdkerrors\.Wrap\(.*ErrUnauthorized'
119-123
: Confirm that accepting ownership while the token factory is paused is valid.The test allows a pending owner to accept ownership even when the fiat token factory is paused. Typically, pausing a module might prevent state-changing operations to ensure system stability. Please verify if accepting ownership during a paused state is consistent with the intended design.
Execute the following script to check if the
AcceptOwner
function enforces any restrictions when the token factory is paused:#!/bin/bash # Description: Check if `AcceptOwner` is restricted when the fiat token factory is paused. # Find the `AcceptOwner` handler implementation. rg --type go 'func (k|c) .*AcceptOwner\(.*\) .* \{' -A 50 # Look for any conditions that check the paused state. rg --type go 'IsPaused\('
44-45
: Check the error message for consistency with the expected authorization error.The test expects an error containing "you are not the owner: unauthorized". Ensure that this error message matches the actual error returned by the
update-owner
action when executed by an unprivileged account. This helps to confirm that the correct error handling is in place.Use the following script to find where the error message is defined:
#!/bin/bash # Description: Locate the error message for unauthorized `update-owner` attempts. # Search for the definition of the authorization error in the fiat token factory module. rg --type go '"you are not the owner: unauthorized"' # Alternatively, check if the standard `sdkerrors.ErrUnauthorized` is used. rg --type go 'sdkerrors\.Wrap\(.*ErrUnauthorized'
160-163
: Verify whether a blacklisted pending owner can accept ownership.In this scenario,
newOwner3
is blacklisted before accepting ownership, yet the acceptance succeeds. Allowing a blacklisted account to assume ownership could pose security risks. Ensure that this behavior is intended and doesn't conflict with the system's security policies.Run the following script to ascertain if the
AcceptOwner
function prevents blacklisted accounts from accepting ownership:#!/bin/bash # Description: Determine if blacklisted accounts are blocked from executing `AcceptOwner`. # Locate the `AcceptOwner` handler implementation. rg --type go 'func (k|c) .*AcceptOwner\(.*\) .* \{' -A 50 # Search for any checks against the caller being blacklisted. rg --type go 'IsBlacklisted\('
50-53
: Verify if blacklisted owners should be able to update the owner.In this test case, the current owner is blacklisted using
BlacklistAccount
, and then attempts to update the owner succeed. Typically, blacklisted accounts are restricted from performing sensitive operations. Please confirm if allowing a blacklisted owner to executeupdate-owner
aligns with the intended security policies.Run the following script to check whether blacklisted accounts can execute the
UpdateOwner
action:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Includes:
TestFiatTFUpdateOwner
TestFiatTFAcceptOwner