Adds revert reason test + error logging #1616
Conversation
WalkthroughThe changes primarily focus on enhancing error handling and testing for Ethereum smart contracts. A new package Changes
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Files selected for processing (4)
- integration/obscurogateway/errors_contract.go (1 hunks)
- integration/obscurogateway/obscurogateway_test.go (3 hunks)
- tools/walletextension/api/routes.go (1 hunks)
- tools/walletextension/api/utils.go (1 hunks)
Files skipped from review due to trivial changes (1)
- tools/walletextension/api/routes.go
Additional comments (Suppressed): 5
tools/walletextension/api/utils.go (3)
118-118: The logging statement has been moved up to log the error response before it is forwarded to the user. This is a good practice as it ensures that the error is logged even if the forwarding fails for any reason.
126-126: The error message in the logging statement has been updated to provide more context about the error. This is a good practice as it makes the logs more informative and easier to understand.
128-130: The error handling in the
handleEthErrorandhandleErrorfunctions has been updated to log an error message if writing the response back to the user fails. This is a good practice as it ensures that all errors are logged and can be investigated if necessary.integration/obscurogateway/obscurogateway_test.go (2)
15-18: The import statement
github.com/ethereum/go-ethereumseems to be unused in the new hunks. Please verify if it's necessary or remove it to avoid unnecessary imports.86-92: The addition of the new test function
testErrorsRevertedArePassedis noted. Ensure that this function is implemented correctly and that it is being called with the correct parameters.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Files selected for processing (1)
- tools/walletextension/api/utils.go (2 hunks)
Additional comments (Suppressed): 4
tools/walletextension/api/utils.go (4)
8-14: The import
github.com/obscuronet/go-obscuro/go/common/errutilhas been added. Ensure that this package is available and accessible in the project's dependencies.112-114: The error is being type-checked and cast to
errutil.EVMSerialisableError. This is a good practice for handling specific error types. However, it's important to ensure that all errors that can be cast toerrutil.EVMSerialisableErrorhave aReasonfield, otherwise this could lead to a runtime panic.130-130: The error message is being logged before writing the response. This is a good practice as it ensures that all errors are logged even if there is a failure when writing the response.
132-134: The error message is being written as the response. This is a good practice for providing feedback to the user. However, ensure that no sensitive information is being exposed in the error messages.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Files selected for processing (3)
- integration/common/utils.go (2 hunks)
- integration/obscurogateway/obscurogateway_test.go (5 hunks)
- tools/walletextension/api/utils.go (2 hunks)
Additional comments (Suppressed): 11
tools/walletextension/api/utils.go (4)
8-13: The import statements have been updated to include the
errutilpackage fromgo-obscuro/go/common. This package is used for handling Ethereum Virtual Machine (EVM) errors.111-114: The error handling has been enhanced to specifically handle
errutil.EVMSerialisableErrortype errors. This is a good practice as it allows for more granular error handling and logging.122-126: The error response is logged before being written back to the client. This is a good practice as it allows for better tracking and debugging of errors.
129-134: The
handleErrorfunction has been updated to log the error before writing the response. This is a good practice as it allows for better tracking and debugging of errors.integration/common/utils.go (2)
12-17: The new imports
github.com/ethereum/go-ethereum/ethclientandgithub.com/obscuronet/go-obscuro/go/common/errutilare introduced to handle Ethereum client and error utility respectively. Ensure these packages are properly installed and available in the project's dependencies.64-88: The new function
AwaitReceiptEthis introduced to wait for a transaction receipt on the Ethereum network. It uses the Ethereum client to fetch the transaction receipt and retries if the transaction is not found. It returns an error if the transaction fails or times out. The function also checks the receipt status and returns an error if the status is failed. This function seems to be a good addition for handling Ethereum transactions and their receipts.69, 76:
The use offmt.Printlnfor logging is not recommended in production code. Consider using a structured logging library that can provide more control over log levels and formats. This will help in filtering and analyzing logs more effectively.- fmt.Println("Fetching receipt for tx: ", txHash.Hex()) + log.Info().Str("txHash", txHash.Hex()).Msg("Fetching receipt for tx") - fmt.Println("No tx receipt after: ", time.Since(startTime)) + log.Info().Dur("timeElapsed", time.Since(startTime)).Msg("No tx receipt after")integration/obscurogateway/obscurogateway_test.go (5)
4-10: The import of
github.com/obscuronet/go-obscuro/go/common/errutilis new. Ensure that this package is available and correctly imported.16-19: The import of
github.com/ethereum/go-ethereumis new. Ensure that this package is available and correctly imported.26-29: The import of
integrationCommon "github.com/obscuronet/go-obscuro/integration/common"is new. Ensure that this package is available and correctly imported.89-94: A new test function
testErrorsRevertedArePassedis added to the test suite. Ensure that this function is implemented correctly and that it is called with the correct parameters.174-244: The new function
testErrorsRevertedArePassedtests the error handling of the Obscuro Gateway when Ethereum smart contract errors are encountered. It deploys a contract, calls functions that force errors, and checks that the errors are correctly handled and returned. This is a good addition to the test suite as it verifies the robustness of the error handling mechanism. However, ensure that the error messages checked in lines 219, 231, and 243 are the expected ones.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Files selected for processing (3)
- integration/obscurogateway/obscurogateway_test.go (6 hunks)
- tools/walletextension/api/utils.go (2 hunks)
- tools/walletextension/common/json.go (1 hunks)
Files skipped from review due to trivial changes (1)
- tools/walletextension/common/json.go
Additional comments (Suppressed): 7
tools/walletextension/api/utils.go (3)
5-13: The new import
github.com/obscuronet/go-obscuro/go/common/errutilis introduced to handle a new type of error (errutil.EVMSerialisableError). Ensure that this package is available and updated in your dependencies.111-114: The error type checking and handling for
errutil.EVMSerialisableErroris a good practice. It allows for more specific error handling and can provide more detailed error information.129-134: The
handleErrorfunction is updated to log the error before writing the response. This is a good practice as it ensures that all errors are logged even if there is a failure in writing the response.integration/obscurogateway/obscurogateway_test.go (4)
22-28: The import statement for the
github.com/obscuronet/go-obscuro/integration/commonpackage is added. Ensure that the new import does not conflict with existing code and is used appropriately.88-91: The
testAreTxsMintedfunction is commented out and a new test functiontestErrorsRevertedArePassedis added. Ensure that the new test function covers all the necessary test cases and does not break existing functionality. Also, iftestAreTxsMintedis no longer needed, consider removing it instead of commenting it out.173-247: A new test function
testErrorsRevertedArePassedis added. This function tests the error handling capabilities of the Obscuro Gateway by making various requests to a Geth server and the Obscuro Gateway server. It compares the responses and ensures that the Obscuro Gateway returns the expected error format. This is a good addition to the test suite as it enhances the error handling tests.270-278: The
AwaitReceiptEthfunction is used to wait for transaction receipts instead of the previous loop-based approach. This is a good change as it simplifies the code and makes it more readable. However, ensure that theAwaitReceiptEthfunction handles all edge cases and timeouts correctly.
Why this change is needed
As per title.
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