-
Notifications
You must be signed in to change notification settings - Fork 5
refactor(withdrawals): enforce preview-then-execute workflow to prevent duplicate signing #276
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
base: dev
Are you sure you want to change the base?
Conversation
…nt duplicate signing - Add executeWithdrawal() method to broadcast pre-signed transactions - Deprecate withdraw() method in favor of two-step preview-execute flow - Update example app to use new executeWithdrawal() method - Prevents transactions from being signed multiple times - Ensures users always review transaction details before broadcast Breaking Changes: - withdraw() is now deprecated (still functional but not recommended) - Recommended flow: previewWithdrawal() → executeWithdrawal()
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Pull Request Overview
This PR introduces a new two-step withdrawal workflow to ensure users review transaction details before broadcasting. The key improvement is separating transaction preview from execution, allowing users to confirm fees and amounts before the transaction is broadcast.
- Adds a new
executeWithdrawal()method that broadcasts pre-signed transactions from previews - Deprecates the direct
withdraw()method in favor of the preview-then-execute workflow - Updates the example app to use the new two-step workflow
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| withdrawal_manager.dart | Adds executeWithdrawal() method and deprecates withdraw() with updated documentation explaining the recommended two-step workflow |
| legacy_withdrawal_manager.dart | Implements executeWithdrawal() for Tendermint assets and deprecates withdraw() |
| withdrawal_page.dart | Updates example to conditionally use the new executeWithdrawal() method when a preview exists, along with code formatting improvements |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/komodo_defi_sdk/lib/src/withdrawals/withdrawal_manager.dart
Outdated
Show resolved
Hide resolved
| // Execute the previewed withdrawal (transaction already signed) | ||
| final progressStream = _preview != null | ||
| ? _sdk.withdrawals.executeWithdrawal(_preview!, params.asset) | ||
| : _sdk.withdrawals.withdraw(params); |
Copilot
AI
Oct 31, 2025
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.
The fallback to withdraw() when _preview is null creates an inconsistent code path. Since _executeWithdrawal() is only called after the preview dialog (which requires _preview to be set), the null check and fallback are unnecessary. Consider either asserting _preview is non-null or restructuring the logic to ensure preview always exists before execution.
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.
@cursoragent apply changes based on this feedback
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.
Perfect! I've applied the feedback by:
- Added an assertion that
_previewmust be non-null before execution - Removed the fallback to
withdraw()method - Simplified the code to directly call
executeWithdrawal()with the preview
This enforces the preview-then-execute workflow more strictly and removes the inconsistent code path. Since _executeWithdrawal() is only called from the preview dialog's confirm button (line 209), and the dialog is only shown after _preview is set (line 166), the assertion will help catch any logic errors while making the code clearer.
Learn more about Cursor Agents
|
Visit the preview URL for this PR (updated for commit b2be41b): https://kdf-sdk--pr276-fix-avoid-multi-sign-itktpglj.web.app (expires Sun, 09 Nov 2025 14:21:26 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 9c1b6e6c010cf0b965c455ba7a69c4aedafa8a1d |
|
Visit the preview URL for this PR (updated for commit b2be41b): https://komodo-playground--pr276-fix-avoid-multi-sign-ehuqsjr3.web.app (expires Sun, 09 Nov 2025 14:22:31 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 2bfedd77fdea45b25ba7c784416e81f177aa5c47 |
…r.dart Co-authored-by: Copilot <[email protected]>
|
Cursor Agent can help with this pull request. Just |
Overview
Refactored SDK withdrawals to enforce a secure preview-then-execute workflow, ensuring transactions are signed only once and users always review what they're signing.
Problem
Previously, when a user previewed a withdrawal and then executed it, the transaction was being signed twice:
This applied to both:
init()twicewithdraw()RPC twiceSolution
Enforced a secure two-step withdrawal workflow:
Changes Made
1. WithdrawalManager (Task-based API)
Added
executeWithdrawal(WithdrawalPreview preview, String assetId)methodinit()again, preventing duplicate signingDeprecated
withdraw(WithdrawParameters)method2. LegacyWithdrawalManager (Tendermint Assets)
Added
executeWithdrawal(WithdrawalPreview preview, String assetId)methodtxHexfrom the previewwithdraw()RPC again, preventing duplicate signingDeprecated
withdraw(WithdrawParameters)method3. Example Implementation
executeWithdrawal()when a preview is availableUsage Pattern
Old (Deprecated) - Signs Transaction Twice ❌
New (Recommended) - Signs Transaction Once ✅
Benefits
Breaking Changes
withdraw()is now deprecated (still works, but not recommended)previewWithdrawal()→executeWithdrawal()Migration Guide
Before:
After:
Testing
Files Modified
packages/komodo_defi_sdk/lib/src/withdrawals/withdrawal_manager.dartpackages/komodo_defi_sdk/lib/src/withdrawals/legacy_withdrawal_manager.dartpackages/komodo_defi_sdk/example/lib/screens/withdrawal_page.dartNote
Introduces
executeWithdrawalto broadcast pre-signed previews, deprecateswithdraw, and updates the example UI to use the preview-then-execute workflow.executeWithdrawal(WithdrawalPreview, String)inlib/src/withdrawals/withdrawal_manager.dartto broadcast pre-signed transactions; ensures asset activation; delegates Tendermint to legacy; streams progress.withdraw(WithdrawParameters)with migration docs to preview→execute.executeWithdrawal(WithdrawalPreview, String)inlib/src/withdrawals/legacy_withdrawal_manager.dartto send raw tx from preview; streams progress.withdraw(WithdrawParameters)with guidance to use preview→execute.example/lib/screens/withdrawal_page.dartto callexecuteWithdrawalwhen aWithdrawalPreviewexists; minor UI/formatting tweaks in preview dialog, fee priority handling, and validation UI.Written by Cursor Bugbot for commit b2be41b. This will update automatically on new commits. Configure here.