Fix/sync dev after v1 52 2 #250
Conversation
WalkthroughThe changes in this pull request focus on enhancing error handling in the Changes
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
Documentation and Community
|
![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 (3)
client/chain/chain.go (3)
Line range hint
836-852: Improved error handling in SyncBroadcastSignedTx methodThe changes in this method enhance the error handling by checking both for errors and non-zero response codes. This is a good improvement as it ensures that transactions that fail during execution (but not during broadcast) are also caught and reported as errors.
However, there's a potential issue to consider:
The error returned on line 839 might not accurately reflect the nature of the failure when
res.TxResponse.Code != 0. Consider creating a custom error that includes both the response code and any error message from the response.Example implementation:
if err != nil || res.TxResponse.Code != 0 { if res.TxResponse.Code != 0 { return res, fmt.Errorf("transaction failed with code %d: %s", res.TxResponse.Code, res.TxResponse.RawLog) } return res, err }This change would provide more detailed error information to the caller.
Line range hint
907-923: Enhanced error handling in broadcastTx methodSimilar to the
SyncBroadcastSignedTxmethod, thebroadcastTxmethod now checks for both errors and non-zero response codes. This is a consistent improvement across both methods.However, there's a similar issue to consider:
The error handling on line 910 could be improved to provide more detailed information about the failure when
res.TxResponse.Code != 0.Suggested improvement:
if err != nil || res.TxResponse.Code != 0 { if res.TxResponse.Code != 0 { return res, fmt.Errorf("transaction failed with code %d: %s", res.TxResponse.Code, res.TxResponse.RawLog) } return res, err }This change would make the error handling consistent with the suggested improvement for
SyncBroadcastSignedTx.Consider extracting the error handling logic into a separate function to avoid code duplication between
SyncBroadcastSignedTxandbroadcastTx. This would improve maintainability and ensure consistent error handling across both methods.Example:
func handleBroadcastResponse(res *txtypes.BroadcastTxResponse, err error) error { if err != nil || res.TxResponse.Code != 0 { if res.TxResponse.Code != 0 { return fmt.Errorf("transaction failed with code %d: %s", res.TxResponse.Code, res.TxResponse.RawLog) } return err } return nil }Then use this function in both methods:
if err := handleBroadcastResponse(res, err); err != nil { return res, err }
Line range hint
1-2500: Overall assessment of changesThe modifications to
SyncBroadcastSignedTxandbroadcastTxmethods improve the error handling by checking both for errors and non-zero response codes. This enhancement ensures that transactions failing during execution are properly caught and reported.However, there are opportunities for further improvement:
- More detailed error reporting when transactions fail with non-zero response codes.
- Potential code duplication in error handling between the two methods.
Consider implementing the suggested improvements to provide more informative error messages and reduce code duplication. These changes would enhance the overall robustness and maintainability of the codebase.
…the TX in the mainpool if it has been rejected by the node during broadcast
PavelInjective
force-pushed
the
fix/sync_dev_after_v1_52_2
branch
from
94c4b2c to
d2c8a23
Compare
Summary by CodeRabbit
Bug Fixes
New Features