Skip to content

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.

Sign up for GitHub

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

Open
CharlVS wants to merge 2 commits into
base: dev
Choose a base branch
from
Open

refactor(withdrawals): enforce preview-then-execute workflow to prevent duplicate signing #276

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6576c4c
refactor(withdrawals): enforce preview-then-execute workflow to preve…
CharlVS Oct 31, 2025
b2be41b
Update packages/komodo_defi_sdk/lib/src/withdrawals/withdrawal_manage…
CharlVS Nov 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
118 changes: 59 additions & 59 deletions packages/komodo_defi_sdk/example/lib/screens/withdrawal_page.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,9 +128,8 @@ class _WithdrawalScreenState extends State<WithdrawalScreen> {

if (!_addressValidation!.isValid) {
setState(
() =>
_error =
_addressValidation!.invalidReason ?? 'Invalid address format',
() => _error =
_addressValidation!.invalidReason ?? 'Invalid address format',
);
return;
}
Expand All @@ -146,17 +145,17 @@ class _WithdrawalScreenState extends State<WithdrawalScreen> {
amount: _isMaxAmount ? null : Decimal.parse(_amountController.text),
fee: _selectedFee,
feePriority: _selectedPriority,
from:
_selectedFromAddress?.derivationPath != null
? WithdrawalSource.hdDerivationPath(
_selectedFromAddress!.derivationPath!,
)
: null,
from: _selectedFromAddress?.derivationPath != null
? WithdrawalSource.hdDerivationPath(
_selectedFromAddress!.derivationPath!,
)
: null,
memo: _memoController.text.isEmpty ? null : _memoController.text,
isMax: _isMaxAmount,
ibcTransfer: _isIbcTransfer ? true : null,
ibcSourceChannel:
_isIbcTransfer ? int.tryParse(_ibcChannelController.text) : null,
ibcSourceChannel: _isIbcTransfer
? int.tryParse(_ibcChannelController.text)
: null,
);

final preview = await _sdk.withdrawals.previewWithdrawal(params);
Expand All @@ -178,48 +177,50 @@ class _WithdrawalScreenState extends State<WithdrawalScreen> {
Future<void> _showPreviewDialog(WithdrawParameters params) async {
return showDialog<void>(
context: context,
builder:
(context) => AlertDialog(
title: const Text('Withdrawal Preview'),
content: Column(
mainAxisSize: MainAxisSize.min,
crossAxisAlignment: CrossAxisAlignment.start,
children: [
Text('Asset: ${params.asset}'),
Text('To: ${params.toAddress}'),
if (params.amount != null)
Text('Amount: ${params.amount} ${params.asset}'),
if (_selectedFee != null) ...[
const SizedBox(height: 8),
FeeInfoDisplay(feeInfo: _selectedFee!),
],
if (_preview != null) ...[
const SizedBox(height: 8),
Text('Estimated fee: ${_preview!.fee.formatTotal()}'),
Text('Balance change: ${_preview!.balanceChanges.netChange}'),
],
],
),
actions: [
TextButton(
onPressed: () => Navigator.of(context).pop(),
child: const Text('Cancel'),
),
FilledButton(
onPressed: () {
Navigator.of(context).pop();
_executeWithdrawal(params);
},
child: const Text('Confirm'),
),
builder: (context) => AlertDialog(
title: const Text('Withdrawal Preview'),
content: Column(
mainAxisSize: MainAxisSize.min,
crossAxisAlignment: CrossAxisAlignment.start,
children: [
Text('Asset: ${params.asset}'),
Text('To: ${params.toAddress}'),
if (params.amount != null)
Text('Amount: ${params.amount} ${params.asset}'),
if (_selectedFee != null) ...[
const SizedBox(height: 8),
FeeInfoDisplay(feeInfo: _selectedFee!),
],
if (_preview != null) ...[
const SizedBox(height: 8),
Text('Estimated fee: ${_preview!.fee.formatTotal()}'),
Text('Balance change: ${_preview!.balanceChanges.netChange}'),
],
],
),
actions: [
TextButton(
onPressed: () => Navigator.of(context).pop(),
child: const Text('Cancel'),
),
FilledButton(
onPressed: () {
Navigator.of(context).pop();
_executeWithdrawal(params);
},
child: const Text('Confirm'),
),
],
),
);
}

Future<void> _executeWithdrawal(WithdrawParameters params) async {
try {
final progressStream = _sdk.withdrawals.withdraw(params);
// Execute the previewed withdrawal (transaction already signed)
final progressStream = _preview != null
? _sdk.withdrawals.executeWithdrawal(_preview!, params.asset)
: _sdk.withdrawals.withdraw(params);
Comment on lines +220 to +223
Copy link

Copilot AI Oct 31, 2025

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.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Collaborator Author

@CharlVS CharlVS Nov 2, 2025

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

Copy link
Copy Markdown

@cursor cursor Bot Nov 2, 2025 •
edited
Loading

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:

  1. Added an assertion that _preview must be non-null before execution
  2. Removed the fallback to withdraw() method
  3. 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.

Open in Cursor Open in Web

Learn more about Cursor Agents


await for (final progress in progressStream) {
if (!mounted) return;
Expand Down Expand Up @@ -349,13 +350,12 @@ class _WithdrawalScreenState extends State<WithdrawalScreen> {
),
CheckboxListTile(
value: _isMaxAmount,
onChanged:
(value) => setState(() {
_isMaxAmount = value == true;
if (_isMaxAmount) {
_amountController.clear();
}
}),
onChanged: (value) => setState(() {
_isMaxAmount = value == true;
if (_isMaxAmount) {
_amountController.clear();
}
}),
title: const Text('Send maximum amount'),
),
const SizedBox(height: 16),
Expand All @@ -368,8 +368,9 @@ class _WithdrawalScreenState extends State<WithdrawalScreen> {
setState(() {
_selectedPriority = priority;
if (_feeOptions != null) {
_selectedFee =
_feeOptions!.getByPriority(priority).feeInfo;
_selectedFee = _feeOptions!
.getByPriority(priority)
.feeInfo;
}
});
},
Expand Down Expand Up @@ -454,10 +455,9 @@ class _WithdrawalScreenState extends State<WithdrawalScreen> {

return Icon(
_addressValidation!.isValid ? Icons.check_circle : Icons.error,
color:
_addressValidation!.isValid
? Theme.of(context).colorScheme.primary
: Theme.of(context).colorScheme.error,
color: _addressValidation!.isValid
? Theme.of(context).colorScheme.primary
: Theme.of(context).colorScheme.error,
);
}

Expand Down
72 changes: 71 additions & 1 deletion packages/komodo_defi_sdk/lib/src/withdrawals/legacy_withdrawal_manager.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,13 @@ class LegacyWithdrawalManager implements WithdrawalManager {

final ApiClient _client;

/// Start a withdrawal operation and return a progress stream
/// Creates a preview and immediately executes the withdrawal.
///
/// **DEPRECATED:** Use [previewWithdrawal] followed by [executeWithdrawal]
/// instead to ensure users can review transaction details before broadcasting.
@Deprecated(
'Use previewWithdrawal() followed by executeWithdrawal() instead.',
)
@override
Stream<WithdrawalProgress> withdraw(WithdrawParameters parameters) async* {
try {
Expand Down Expand Up @@ -125,6 +131,70 @@ class LegacyWithdrawalManager implements WithdrawalManager {
}
}

/// Execute a withdrawal from a previously generated preview.
///
/// This method broadcasts the pre-signed transaction from the preview,
/// avoiding the need to sign the transaction again. This is the ONLY
/// recommended way to execute withdrawals for Tendermint assets.
///
/// Parameters:
/// - [preview] - The preview result from [previewWithdrawal]
/// - [assetId] - The asset identifier (coin symbol)
///
/// Returns a [Stream<WithdrawalProgress>] that emits progress updates.
@override
Stream<WithdrawalProgress> executeWithdrawal(
WithdrawalPreview preview,
String assetId,
) async* {
try {
// Initial progress update
yield WithdrawalProgress(
status: WithdrawalStatus.inProgress,
message: 'Broadcasting signed transaction...',
withdrawalResult: WithdrawalResult(
txHash: preview.txHash,
balanceChanges: preview.balanceChanges,
coin: assetId,
toAddress: preview.to.first,
fee: preview.fee,
kmdRewardsEligible:
preview.kmdRewards != null &&
Decimal.parse(preview.kmdRewards!.amount) > Decimal.zero,
),
);

// Broadcast the pre-signed transaction
final broadcastResponse = await _client.rpc.withdraw.sendRawTransaction(
coin: assetId,
txHex: preview.txHex,
);

// Final success update with actual broadcast transaction hash
yield WithdrawalProgress(
status: WithdrawalStatus.complete,
message: 'Withdrawal completed successfully',
withdrawalResult: WithdrawalResult(
txHash: broadcastResponse.txHash,
balanceChanges: preview.balanceChanges,
coin: assetId,
toAddress: preview.to.first,
fee: preview.fee,
kmdRewardsEligible:
preview.kmdRewards != null &&
Decimal.parse(preview.kmdRewards!.amount) > Decimal.zero,
),
);
} catch (e) {
yield* Stream.error(
WithdrawalException(
'Failed to broadcast transaction: $e',
WithdrawalErrorCode.networkError,
),
);
}
}

/// No-op for legacy implementation since there's no task to cancel
@override
Future<bool> cancelWithdrawal(int taskId) async => false;
Expand Down
Loading
Loading