-
Notifications
You must be signed in to change notification settings - Fork 38
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
Adding Tx Hash to the faucet funding #1660
Conversation
WalkthroughThe changes across the codebase involve enhancing the Changes
TipsChat with CodeRabbit Bot (
|
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.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (3)
- integration/faucet/faucet_test.go (2 hunks)
- tools/faucet/faucet/faucet.go (1 hunks)
- tools/faucet/webserver/web_server.go (1 hunks)
Additional comments: 2
integration/faucet/faucet_test.go (2)
111-117: The URL in the
fundWallet
function has been updated to include/auth
in the path, indicating that the endpoint now requires authentication. Ensure that the corresponding endpoint in the server has been updated to handle this new authenticated route.125-125: An "Authorization" header with a mock JWT token has been added to the HTTP request in the
fundWallet
function. This change implies that the faucet service is now using JWT for authentication. Verify that the JWT token used here aligns with the authentication mechanism and that the token is valid for the test environment.
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.
lgtm
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.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- tools/faucet/faucet/faucet.go (1 hunks)
Additional comments: 2
tools/faucet/faucet/faucet.go (2)
- 54-80: - The
Fund
method has been correctly updated to return a transaction hash as a string along with an error. This aligns with the change summary.
- Ensure that all callers of the
Fund
method are updated to handle the new return type(string, error)
.- The error message in line 77 includes the transaction hash, which is a good practice for debugging purposes.
- The logging statement in line 73 is informative, but consider if sensitive information is being logged.
- The
validateTx
method is called after the transaction is sent, which is a good practice to ensure the transaction was successful. However, there is atodo
comment on line 74 regarding handling the transaction receipt which should be addressed before finalizing the code.- The
fundNativeToken
method is called within a mutex lock, which is good for preventing race conditions when multiple funding operations occur.
- 83-83: - The
validateTx
method is not part of the hunk, but it's important to ensure that the validation logic is robust and handles all possible cases.
- The method waits for the transaction receipt with a timeout, which is a good practice to avoid indefinite waiting. However, the error message "tx status is not 0x1" could be more descriptive, indicating that the transaction failed.
Why this change is needed
Adds the tx hash to the reply from the faucet.
Updates test to use the Authenticated path .
What changes were made as part of this PR
Please provide a high level list of the changes made
PR checks pre-merging
Please indicate below by ticking the checkbox that you have read and performed the required
PR checks