fix(withdrawal-manager): use legacy RPCs for tendermint withdrawals#57
fix(withdrawal-manager): use legacy RPCs for tendermint withdrawals#57
Conversation
PlatformIsAlreadyActivated error seen in SDK example and KW. Ideal solution is to retry in the SDK before throwing an exception
|
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 WalkthroughThis update introduces dependency overrides for Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant WithdrawalManager
participant Asset
participant LegacyWithdrawalManager
User->>WithdrawalManager: request withdrawal (assetConfigId)
WithdrawalManager->>Asset: get asset by configId
WithdrawalManager->>Asset: check protocol type
alt Asset protocol is Tendermint
WithdrawalManager->>LegacyWithdrawalManager: instantiate and call withdraw/previewWithdrawal
LegacyWithdrawalManager-->>User: return result
else Non-Tendermint asset
WithdrawalManager->>WithdrawalManager: proceed with task-based withdrawal logic
WithdrawalManager-->>User: return result
end
sequenceDiagram
participant PubkeyManager
participant ActivationManager
PubkeyManager->>retryWithBackoff: call with activation function
loop Until success or max attempts
retryWithBackoff->>ActivationManager: activateAsset(asset)
alt Activation succeeds
retryWithBackoff-->>PubkeyManager: return result
else Activation fails and should retry
retryWithBackoff->>retryWithBackoff: wait (exponential backoff)
else Activation fails and should not retry
retryWithBackoff-->>PubkeyManager: throw error
end
end
Poem
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
CodeRabbit Configuration File (
|
|
Visit the preview URL for this PR (updated for commit 8d9c80a): https://komodo-defi-sdk--pr57-bugfix-tendermint-wi-nyi0yufa.web.app (expires Wed, 28 May 2025 12:53:20 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 7f9f5ac39928f333b6e8fcefb7138575e24ed347 |
exceptions thrown in microtask queues are not caught by traditional try-catch blocks (even when awaited), so run the function to be retried in a guarded zone to catch all exceptions
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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
🧹 Nitpick comments (2)
packages/komodo_defi_framework/pubspec_overrides.yaml (1)
3-4: Add local override forkomodo_coinsA new dependency override for
komodo_coinswithpath: ../komodo_coinshas been introduced. The path looks correct relative to this package. For clarity, you may want to order the overrides to match the header comment’s sequence or sort them alphabetically.packages/komodo_defi_local_auth/pubspec_overrides.yaml (1)
3-4: Addkomodo_coinsdependency overrideThe override for
komodo_coinspointing to../komodo_coinshas been added. Verify that this path resolves correctly and consider reordering the entries to match the header comment’s sequence or for alphabetical consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (6)
packages/komodo_defi_framework/pubspec_overrides.yaml(1 hunks)packages/komodo_defi_local_auth/pubspec_overrides.yaml(1 hunks)packages/komodo_defi_sdk/lib/src/pubkeys/pubkey_manager.dart(2 hunks)packages/komodo_defi_sdk/lib/src/withdrawals/legacy_withdrawal_manager.dart(3 hunks)packages/komodo_defi_sdk/lib/src/withdrawals/withdrawal_manager.dart(3 hunks)packages/komodo_defi_types/lib/src/transactions/fee_info.dart(1 hunks)
🔇 Additional comments (12)
packages/komodo_defi_framework/pubspec_overrides.yaml (1)
1-1: Includekomodo_coinsin Melos-managed dependencies commentThe header comment
# melos_managed_dependency_overrideshas been updated to listkomodo_coins, matching the added override entry below. This ensures Melos will track it properly.packages/komodo_defi_local_auth/pubspec_overrides.yaml (1)
1-1: Update Melos-managed overrides headerThe
# melos_managed_dependency_overridescomment now includeskomodo_coins, reflecting the new override. This keeps the list in sync with the actual overrides.packages/komodo_defi_sdk/lib/src/pubkeys/pubkey_manager.dart (3)
6-58: Well-implemented retry utility with exponential backoffThe
retryWithBackoffutility function is a solid implementation of the exponential backoff pattern. It handles microtask errors properly by usingrunZonedGuardedand provides flexibility with two retry predicates.A few strengths of this implementation:
- Clear distinction between retry conditions that should/shouldn't increment the attempt counter
- Error propagation via completion of a completer rather than just throwing
- Exponential delay increase with each retry attempt
70-70: Good integration of retry mechanismReplacing the direct activation call with the retry utility improves the robustness of the pubkey retrieval process, especially for network-related operations that might experience transient failures.
77-77: Consistent application of retry patternApplying the same retry pattern to
createNewPubkeyensures a consistent approach to handling potential activation failures across the codebase.packages/komodo_defi_sdk/lib/src/withdrawals/legacy_withdrawal_manager.dart (3)
48-50: Code formatting improvementThe improved formatting makes the code more readable while maintaining the same logic.
71-73: Consistent formatting applicationApplying the same formatting improvements consistently throughout the codebase.
117-117: Enhanced error handling with stack trace captureAdding the stack trace (
s) to the catch block is a good practice for better debugging and error reporting. This will help with troubleshooting issues that might occur during withdrawal previews.packages/komodo_defi_types/lib/src/transactions/fee_info.dart (1)
50-59: Good backward compatibility for legacy APIAdding support for the "Tendermint" fee type ensures compatibility with the legacy withdraw API. The implementation properly maps it to the existing
cosmosGasvariant while handling the different field names in the JSON payload.The comments explaining the rationale are helpful for future maintainers.
packages/komodo_defi_sdk/lib/src/withdrawals/withdrawal_manager.dart (3)
6-6: Required import for the new featureThe import of
legacy_withdrawal_manager.dartis necessary for the new conditional handling of Tendermint protocol withdrawals.
46-55: Clean implementation of Tendermint protocol delegationThe implementation correctly identifies assets using the Tendermint protocol and delegates their withdrawal preview to the legacy manager. This separation of concerns is well-structured and maintains a clean code organization.
100-110: Consistent implementation for withdrawal executionThe same pattern used in
previewWithdrawalis applied consistently to thewithdrawmethod, creating a unified approach to handling Tendermint protocol assets.
| /// Retry utility with exponential backoff. | ||
| /// If [shouldRetry] returns true, the attempt counter is incremented. | ||
| /// If [shouldRetryNoIncrement] returns true, the attempt counter is NOT | ||
| /// incremented. Use with caution. The intended application is for | ||
| /// false positives, where the error is not a failure of the function | ||
| /// E.g. PlatformIsAlreadyActivated | ||
| Future<T> retryWithBackoff<T>( | ||
| Future<T> Function() fn, { | ||
| int maxAttempts = 5, | ||
| Duration initialDelay = const Duration(milliseconds: 200), | ||
| bool Function(Object error)? shouldRetry, | ||
| bool Function(Object error)? shouldRetryNoIncrement, | ||
| }) async { | ||
| var attempt = 0; | ||
| var delay = initialDelay; | ||
|
|
||
| while (true) { | ||
| final completer = Completer<T>(); | ||
|
|
||
| // RPC calls are scheduled microtasks, so we need to run them in a zone | ||
| // to catch errors that are thrown in the microtask queue, which would | ||
| // otherwise be unhandled. | ||
| await runZonedGuarded( | ||
| () async { | ||
| final result = await fn(); | ||
| if (!completer.isCompleted) { | ||
| completer.complete(result); | ||
| } | ||
| }, | ||
| (error, stack) { | ||
| if (!completer.isCompleted) { | ||
| completer.completeError(error, stack); | ||
| } | ||
| }, | ||
| ); | ||
|
|
||
| try { | ||
| return await completer.future; | ||
| } catch (e) { | ||
| if (shouldRetryNoIncrement != null && shouldRetryNoIncrement(e)) { | ||
| await Future<void>.delayed(delay); | ||
| delay *= 2; | ||
| continue; | ||
| } | ||
| attempt++; | ||
| if (attempt >= maxAttempts || (shouldRetry != null && !shouldRetry(e))) { | ||
| rethrow; | ||
| } | ||
| await Future<void>.delayed(delay); | ||
| delay *= 2; | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Since this isn't specific to the SDK or the pubkey manager, please move this to the types utils in the types package.
2 participants
Fixes the following
PlatformIsAlreadyActivated, requiring the user to navigate back to the coins list to click on the coin again to see coin details.Changes
WithdrawalManagerto useLegacyWithdrawalManagerfor Tendermint protocol assetsPubkeyManagerto resolve coin activation failures in KW and the example preview. This is usually due to thePlatformIsAlreadyActivatederror returned because of race conditions with auto-activation. Balance watching requests generally activate the coin before thegetPubkeysinAssetPage, resulting in thegetPubkeysfunction throwing an exception that is treated as fatal in KW and the SDK example project.Summary by CodeRabbit
New Features
Chores
Style