Waiting for receipt in simulation tests. #1571
Conversation
integration/common/utils.go
Outdated
| } | ||
|
|
||
| if receipt.Status == types.ReceiptStatusFailed { | ||
| return fmt.Errorf("receipt had status failed for transaction %s", txHash.Hex()) |
There was a problem hiding this comment.
why is this an error?
integration/simulation/simulation.go
Outdated
| } | ||
|
|
||
| time.Sleep(3 * time.Second) | ||
| // time.Sleep(3 * time.Second) |
integration/common/utils.go
Outdated
| @@ -36,6 +38,28 @@ func RndBtwTime(min time.Duration, max time.Duration) time.Duration { | |||
| return time.Duration(RndBtw(uint64(min.Nanoseconds()), uint64(max.Nanoseconds()))) * time.Nanosecond | |||
| } | |||
|
|
|||
| func AwaitReceiptEth(client ethadapter.EthClient, txHash gethcommon.Hash, timeout time.Duration) error { | |||
There was a problem hiding this comment.
There's an existingAwaitReceiptEth method that we can potentially use instead.
|
@StefanIliev545 , what's the status of this PR? |
I haven't looked at it, but I think @otherview played with the sleep this PR is removing so maybe we can close it? Or am I remembering something else? |
Yeah I did played around with it but didn't reach a conclusion, ended up not changing anything.. ! |
|
@StefanIliev545 , close or merge? |
WalkthroughThe recent update involves modifying the way Ethereum transactions are handled and awaited upon. A new, possibly more efficient or secure client ( Changes
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 X ? 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: 7
Configuration used: CodeRabbit UI
Files selected for processing (2)
- integration/common/utils.go (1 hunks)
- integration/simulation/simulation.go (1 hunks)
Additional comments: 20
integration/common/utils.go (15)
67-67: The
AwaitReceiptfunction has been correctly updated to use thecontext.Contextand*obsclient.AuthObsClient. This aligns with the PR objectives to improve the handling of transaction receipts in simulation tests.67-67: The
AwaitReceiptfunction's error handling forrpc.ErrNilResponseseems to be missing in the new implementation. If this is intentional due to changes in theclient.TransactionReceiptmethod's behavior, it's fine. Otherwise, this might be an oversight that could lead to unhandled errors.+ if err != nil && !errors.Is(err, rpc.ErrNilResponse) && !errors.Is(err, errutil.ErrNotFound) { + // we only retry for a nil "not found" response. This is a different error, so we bail out of the retry loop + return retry.FailFast(err) + }
59-59: The error message for a failed transaction receipt status is clear and informative. However, ensure that this error handling is consistent with the rest of the codebase and that the failed status is handled as expected in all calling functions.
65-65: The
AwaitReceiptEthfunction is still present and has been updated to usecontext.Context. This seems to contradict the AI-generated summary which mentioned the removal of this function. If this function is still required, ensure that all references to the oldAwaitReceiptEthare updated to use this new signature.65-65: The
AwaitReceiptEthfunction now returns a*types.Receiptalong with an error. This is a change from the previous version which only returned an error. Ensure that all callers of this function are updated to handle the returned receipt.65-65: The
AwaitReceiptEthfunction usesretry.Dowith aretry.NewTimeoutStrategy. Ensure that the retry strategy is consistent with the desired behavior and that the timeout values are configured appropriately for the expected transaction confirmation times.65-65: The
AwaitReceiptEthfunction checks forreceipt.Status == types.ReceiptStatusFailedand returns an error if the status is failed. This is good practice as it ensures that only successful transactions are considered valid. Ensure that this check is consistent with the rest of the codebase.65-65: The
AwaitReceiptEthfunction's error message for a failed receipt status includes the transaction hash. This is helpful for debugging and is a good practice.65-65: The
AwaitReceiptEthfunction's error handling seems to be inconsistent with theAwaitReceiptfunction. TheAwaitReceiptfunction useserrors.Is(err, rpc.ErrNilResponse), whileAwaitReceiptEthuseserrors.Is(err, errutil.ErrNotFound). Ensure that the error handling is consistent and correct for the context in which each function is used.+ if err != nil && !errors.Is(err, rpc.ErrNilResponse) { + // we only retry for a nil "not found" response. This is a different error, so we bail out of the retry loop + return retry.FailFast(err) + }
65-65: The
AwaitReceiptEthfunction's error message for a failed transaction receipt status is clear and informative. However, ensure that this error handling is consistent with the rest of the codebase and that the failed status is handled as expected in all calling functions.65-65: The
AwaitReceiptEthfunction now returns a*types.Receiptalong with an error. This is a change from the previous version which only returned an error. Ensure that all callers of this function are updated to handle the returned receipt.65-65: The
AwaitReceiptEthfunction usesretry.Dowith aretry.NewTimeoutStrategy. Ensure that the retry strategy is consistent with the desired behavior and that the timeout values are configured appropriately for the expected transaction confirmation times.65-65: The
AwaitReceiptEthfunction checks forreceipt.Status == types.ReceiptStatusFailedand returns an error if the status is failed. This is good practice as it ensures that only successful transactions are considered valid. Ensure that this check is consistent with the rest of the codebase.65-65: The
AwaitReceiptEthfunction's error message for a failed receipt status includes the transaction hash. This is helpful for debugging and is a good practice.65-65: The
AwaitReceiptEthfunction's error handling seems to be inconsistent with theAwaitReceiptfunction. TheAwaitReceiptfunction useserrors.Is(err, rpc.ErrNilResponse), whileAwaitReceiptEthuseserrors.Is(err, errutil.ErrNotFound). Ensure that the error handling is consistent and correct for the context in which each function is used.+ if err != nil && !errors.Is(err, rpc.ErrNilResponse) { + // we only retry for a nil "not found" response. This is a different error, so we bail out of the retry loop + return retry.FailFast(err) + }integration/simulation/simulation.go (5)
148-175: The changes to
bridgeFundingToObscurofunction include the introduction of atransactionsslice and the use oftestcommon.AwaitReceiptEthwith a context and timeout. Ensure that thetransactionsslice is used appropriately throughout the codebase and that the replacement oftestcommon.AwaitReceiptEthaligns with the updatedAwaitReceiptfunction inutils.go.148-175: The
bridgeFundingToObscurofunction uses a wait group to wait for all transactions to be processed. This is a good practice for concurrent operations. Ensure that the wait group is used correctly and that all goroutines are properly managed.148-175: The
bridgeFundingToObscurofunction appends transactions to thetransactionsslice and then processes them concurrently. Ensure that the slice is not being modified concurrently, which could lead to race conditions.sync.Mutex // Consider using a mutex to protect the slice during concurrent access
148-175: The
bridgeFundingToObscurofunction uses a wait group to wait for all transactions to be processed. This is a good practice for concurrent operations. Ensure that the wait group is used correctly and that all goroutines are properly managed.148-175: The
bridgeFundingToObscurofunction appends transactions to thetransactionsslice and then processes them concurrently. Ensure that the slice is not being modified concurrently, which could lead to race conditions.sync.Mutex // Consider using a mutex to protect the slice during concurrent access
integration/common/utils.go
Outdated
| /* | ||
| func AwaitReceiptEth(client ethadapter.EthClient, txHash gethcommon.Hash, timeout time.Duration) error { | ||
| var receipt *types.Receipt | ||
| var err error | ||
| err = retry.Do(func() error { | ||
| receipt, err = client.TransactionReceipt(txHash) | ||
| if err != nil && !errors.Is(err, rpc.ErrNilResponse) && !errors.Is(err, ethereum.NotFound) { | ||
| // we only retry for a nil "not found" response. This is a different error, so we bail out of the retry loop | ||
| return retry.FailFast(err) | ||
| } | ||
| return err | ||
| }, retry.NewTimeoutStrategy(timeout, _awaitReceiptPollingInterval)) | ||
| if err != nil { | ||
| return fmt.Errorf("could not retrieve receipt for transaction %s - %w", txHash.Hex(), err) | ||
| } | ||
|
|
||
| if receipt.Status == types.ReceiptStatusFailed { | ||
| return fmt.Errorf("receipt had status failed for transaction %s", txHash.Hex()) | ||
| } | ||
|
|
||
| return nil | ||
| }*/ |
There was a problem hiding this comment.
The AwaitReceiptEth function is commented out. If this function is not intended for use and is being replaced by AwaitReceipt, it should be removed to avoid confusion and maintain code cleanliness.
- /*
- func AwaitReceiptEth(client ethadapter.EthClient, txHash gethcommon.Hash, timeout time.Duration) error {
- ...
- }*/Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| /* | |
| func AwaitReceiptEth(client ethadapter.EthClient, txHash gethcommon.Hash, timeout time.Duration) error { | |
| var receipt *types.Receipt | |
| var err error | |
| err = retry.Do(func() error { | |
| receipt, err = client.TransactionReceipt(txHash) | |
| if err != nil && !errors.Is(err, rpc.ErrNilResponse) && !errors.Is(err, ethereum.NotFound) { | |
| // we only retry for a nil "not found" response. This is a different error, so we bail out of the retry loop | |
| return retry.FailFast(err) | |
| } | |
| return err | |
| }, retry.NewTimeoutStrategy(timeout, _awaitReceiptPollingInterval)) | |
| if err != nil { | |
| return fmt.Errorf("could not retrieve receipt for transaction %s - %w", txHash.Hex(), err) | |
| } | |
| if receipt.Status == types.ReceiptStatusFailed { | |
| return fmt.Errorf("receipt had status failed for transaction %s", txHash.Hex()) | |
| } | |
| return nil | |
| }*/ |
integration/common/utils.go
Outdated
|
|
||
| return nil | ||
| }*/ | ||
|
|
||
| // AwaitReceipt blocks until the receipt for the transaction with the given hash has been received. Errors if the |
There was a problem hiding this comment.
The AwaitReceiptEth function prints to the console with fmt.Println. This is not a standard approach for logging in production code and could be replaced with a structured logging framework that allows for log levels and better control over log output.
- fmt.Println("Fetching receipt for tx: ", txHash.Hex())
- fmt.Println("No tx receipt after: ", time.Since(startTime))
+ // Use structured logging instead
+ log.Info("Fetching receipt for tx", "txHash", txHash.Hex())
+ log.Info("No tx receipt after", "duration", time.Since(startTime))Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| // AwaitReceipt blocks until the receipt for the transaction with the given hash has been received. Errors if the | |
| // AwaitReceipt blocks until the receipt for the transaction with the given hash has been received. Errors if the | |
| // Use structured logging instead | |
| log.Info("Fetching receipt for tx", "txHash", txHash.Hex()) | |
| log.Info("No tx receipt after", "duration", time.Since(startTime)) |
The AwaitReceiptEth function's use of fmt.Println for logging is not ideal. Consider using a structured logging approach for better log management and consistency across the codebase.
- fmt.Println("Fetching receipt for tx: ", txHash.Hex())
- fmt.Println("No tx receipt after: ", time.Since(startTime))
+ // Replace with structured logging
+ log.Info("Fetching receipt for tx", "txHash", txHash.Hex())
+ log.Info("No tx receipt after", "duration", time.Since(startTime))Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| // AwaitReceipt blocks until the receipt for the transaction with the given hash has been received. Errors if the | |
| // AwaitReceipt blocks until the receipt for the transaction with the given hash has been received. Errors if the | |
| // Replace with structured logging | |
| log.Info("Fetching receipt for tx", "txHash", txHash.Hex()) | |
| log.Info("No tx receipt after", "duration", time.Since(startTime)) |
The AwaitReceiptEth function prints to the console with fmt.Println. This is not a standard approach for logging in production code and could be replaced with a structured logging framework that allows for log levels and better control over log output.
- fmt.Println("Fetching receipt for tx: ", txHash.Hex())
- fmt.Println("No tx receipt after: ", time.Since(startTime))
+ // Use structured logging instead
+ log.Info("Fetching receipt for tx", "txHash", txHash.Hex())
+ log.Info("No tx receipt after", "duration", time.Since(startTime))Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| // AwaitReceipt blocks until the receipt for the transaction with the given hash has been received. Errors if the | |
| // AwaitReceipt blocks until the receipt for the transaction with the given hash has been received. Errors if the | |
| // Use structured logging instead | |
| log.Info("Fetching receipt for tx", "txHash", txHash.Hex()) | |
| log.Info("No tx receipt after", "duration", time.Since(startTime)) |
The AwaitReceiptEth function's use of fmt.Println for logging is not ideal. Consider using a structured logging approach for better log management and consistency across the codebase.
- fmt.Println("Fetching receipt for tx: ", txHash.Hex())
- fmt.Println("No tx receipt after: ", time.Since(startTime))
+ // Replace with structured logging
+ log.Info("Fetching receipt for tx", "txHash", txHash.Hex())
+ log.Info("No tx receipt after", "duration", time.Since(startTime))Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| // AwaitReceipt blocks until the receipt for the transaction with the given hash has been received. Errors if the | |
| // AwaitReceipt blocks until the receipt for the transaction with the given hash has been received. Errors if the | |
| // Replace with structured logging | |
| log.Info("Fetching receipt for tx", "txHash", txHash.Hex()) | |
| log.Info("No tx receipt after", "duration", time.Since(startTime)) |
| transactions := make(types.Transactions, 0) | ||
| for idx, w := range wallets { | ||
| opts, err := bind.NewKeyedTransactorWithChainID(w.PrivateKey(), w.ChainID()) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| opts.Value = value | ||
|
|
||
| _, err = busCtr.SendValueToL2(opts, receivers[idx], value) | ||
| tx, err := busCtr.SendValueToL2(opts, receivers[idx], value) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| transactions = append(transactions, tx) | ||
| } | ||
|
|
||
| time.Sleep(15 * time.Second) | ||
| // todo - fix the wait group, for whatever reason it does not find a receipt... | ||
| /*wg := sync.WaitGroup{} | ||
| wg := sync.WaitGroup{} | ||
| for _, tx := range transactions { | ||
| wg.Add(1) | ||
| transaction := tx | ||
| go func() { | ||
| defer wg.Done() | ||
| err := testcommon.AwaitReceiptEth(s.ctx, s.RPCHandles.RndEthClient(), transaction.Hash(), 20*time.Second) | ||
| _, err := testcommon.AwaitReceiptEth(context.Background(), s.RPCHandles.RndEthClient().EthClient(), transaction.Hash(), 2*time.Minute) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| }() | ||
| } | ||
| wg.Wait()*/ | ||
| wg.Wait() |
There was a problem hiding this comment.
The use of panic for error handling in the bridgeFundingToObscuro function is not ideal for production code. Consider using error returns and handling them gracefully to avoid crashing the entire application.
- panic(err)
+ // Handle error without panicking
+ return fmt.Errorf("error while sending value to L2: %w", err)Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| transactions := make(types.Transactions, 0) | |
| for idx, w := range wallets { | |
| opts, err := bind.NewKeyedTransactorWithChainID(w.PrivateKey(), w.ChainID()) | |
| if err != nil { | |
| panic(err) | |
| } | |
| opts.Value = value | |
| _, err = busCtr.SendValueToL2(opts, receivers[idx], value) | |
| tx, err := busCtr.SendValueToL2(opts, receivers[idx], value) | |
| if err != nil { | |
| panic(err) | |
| } | |
| transactions = append(transactions, tx) | |
| } | |
| time.Sleep(15 * time.Second) | |
| // todo - fix the wait group, for whatever reason it does not find a receipt... | |
| /*wg := sync.WaitGroup{} | |
| wg := sync.WaitGroup{} | |
| for _, tx := range transactions { | |
| wg.Add(1) | |
| transaction := tx | |
| go func() { | |
| defer wg.Done() | |
| err := testcommon.AwaitReceiptEth(s.ctx, s.RPCHandles.RndEthClient(), transaction.Hash(), 20*time.Second) | |
| _, err := testcommon.AwaitReceiptEth(context.Background(), s.RPCHandles.RndEthClient().EthClient(), transaction.Hash(), 2*time.Minute) | |
| if err != nil { | |
| panic(err) | |
| } | |
| }() | |
| } | |
| wg.Wait()*/ | |
| wg.Wait() | |
| transactions := make(types.Transactions, 0) | |
| for idx, w := range wallets { | |
| opts, err := bind.NewKeyedTransactorWithChainID(w.PrivateKey(), w.ChainID()) | |
| if err != nil { | |
| // Handle error without panicking | |
| return fmt.Errorf("error while creating transactor: %w", err) | |
| } | |
| opts.Value = value | |
| tx, err := busCtr.SendValueToL2(opts, receivers[idx], value) | |
| if err != nil { | |
| // Handle error without panicking | |
| return fmt.Errorf("error while sending value to L2: %w", err) | |
| } | |
| transactions = append(transactions, tx) | |
| } | |
| wg := sync.WaitGroup{} | |
| for _, tx := range transactions { | |
| wg.Add(1) | |
| transaction := tx | |
| go func() { | |
| defer wg.Done() | |
| _, err := testcommon.AwaitReceiptEth(context.Background(), s.RPCHandles.RndEthClient().EthClient(), transaction.Hash(), 2*time.Minute) | |
| if err != nil { | |
| // Handle error without panicking | |
| return fmt.Errorf("error while waiting for receipt: %w", err) | |
| } | |
| }() | |
| } | |
| wg.Wait() |
The bridgeFundingToObscuro function's use of panic for error handling is not ideal. Consider using error returns and handling them gracefully to avoid crashing the entire application.
- panic(err)
+ // Handle error without panicking
+ return fmt.Errorf("error while sending value to L2: %w", err)Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| transactions := make(types.Transactions, 0) | |
| for idx, w := range wallets { | |
| opts, err := bind.NewKeyedTransactorWithChainID(w.PrivateKey(), w.ChainID()) | |
| if err != nil { | |
| panic(err) | |
| } | |
| opts.Value = value | |
| _, err = busCtr.SendValueToL2(opts, receivers[idx], value) | |
| tx, err := busCtr.SendValueToL2(opts, receivers[idx], value) | |
| if err != nil { | |
| panic(err) | |
| } | |
| transactions = append(transactions, tx) | |
| } | |
| time.Sleep(15 * time.Second) | |
| // todo - fix the wait group, for whatever reason it does not find a receipt... | |
| /*wg := sync.WaitGroup{} | |
| wg := sync.WaitGroup{} | |
| for _, tx := range transactions { | |
| wg.Add(1) | |
| transaction := tx | |
| go func() { | |
| defer wg.Done() | |
| err := testcommon.AwaitReceiptEth(s.ctx, s.RPCHandles.RndEthClient(), transaction.Hash(), 20*time.Second) | |
| _, err := testcommon.AwaitReceiptEth(context.Background(), s.RPCHandles.RndEthClient().EthClient(), transaction.Hash(), 2*time.Minute) | |
| if err != nil { | |
| panic(err) | |
| } | |
| }() | |
| } | |
| wg.Wait()*/ | |
| wg.Wait() | |
| transactions := make(types.Transactions, 0) | |
| for idx, w := range wallets { | |
| opts, err := bind.NewKeyedTransactorWithChainID(w.PrivateKey(), w.ChainID()) | |
| if err != nil { | |
| // Handle error without panicking | |
| return fmt.Errorf("error while sending value to L2: %w", err) | |
| } | |
| opts.Value = value | |
| tx, err := busCtr.SendValueToL2(opts, receivers[idx], value) | |
| if err != nil { | |
| // Handle error without panicking | |
| return fmt.Errorf("error while sending value to L2: %w", err) | |
| } | |
| transactions = append(transactions, tx) | |
| } | |
| wg := sync.WaitGroup{} | |
| for _, tx := range transactions { | |
| wg.Add(1) | |
| transaction := tx | |
| go func() { | |
| defer wg.Done() | |
| _, err := testcommon.AwaitReceiptEth(context.Background(), s.RPCHandles.RndEthClient().EthClient(), transaction.Hash(), 2*time.Minute) | |
| if err != nil { | |
| // Handle error without panicking | |
| return fmt.Errorf("error while sending value to L2: %w", err) | |
| } | |
| }() | |
| } | |
| wg.Wait() |
Why this change is needed
Please provide a description and a link to the underlying ticket
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