feat(sia): add remaining fields and align with docs #297
Conversation
|
Important Review skippedDraft detected. 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. WalkthroughThis pull request enhances SIA protocol support with task-based activation management, adds password field to SIA activation parameters, refactors SIA withdrawal to use legacy transaction paths, and introduces new RPC methods for task status, cancellation, and user action handling. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SiaMethodsNamespace
participant SiaActivationStrategy
participant RPC as RPC Layer
Client->>SiaMethodsNamespace: enableSiaInit(ticker, params)
SiaMethodsNamespace->>RPC: execute(TaskEnableSiaInit)
RPC-->>SiaMethodsNamespace: NewTaskResponse with taskId
SiaMethodsNamespace-->>Client: return taskId
Client->>SiaActivationStrategy: activate(taskId)
loop Polling (every pollingInterval)
SiaActivationStrategy->>SiaMethodsNamespace: enableSiaStatus(taskId)
SiaMethodsNamespace->>RPC: execute(TaskEnableSiaStatus)
RPC-->>SiaActivationStrategy: status response
alt Status is Complete
SiaActivationStrategy->>Client: emit ActivationProgress(complete)
else Status is Processing
SiaActivationStrategy->>Client: emit ActivationProgress(in-progress)
else Status is Error
SiaActivationStrategy->>Client: emit ActivationProgress(failed)
end
end
alt User Action Required (e.g., PIN entry)
Client->>SiaMethodsNamespace: enableSiaUserAction(taskId, actionType, pin)
SiaMethodsNamespace->>RPC: execute(TaskEnableSiaUserAction)
RPC-->>Client: UserActionResponse
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 |
|
Visit the preview URL for this PR (updated for commit 473c371): https://kdf-sdk--pr297-refactor-add-remaini-c52s6bcx.web.app (expires Mon, 01 Dec 2025 07:48:42 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 9c1b6e6c010cf0b965c455ba7a69c4aedafa8a1d |
|
Visit the preview URL for this PR (updated for commit 473c371): https://komodo-playground--pr297-refactor-add-remaini-2axnjzfz.web.app (expires Mon, 01 Dec 2025 07:50:33 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 2bfedd77fdea45b25ba7c784416e81f177aa5c47 |
takenagain
changed the title
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
...pc_methods/lib/src/common_structures/activation/activation_params/sia_activation_params.dart
Show resolved
Hide resolved
.../komodo_defi_rpc_methods/lib/src/common_structures/transaction_history/transaction_info.dart
Show resolved
Hide resolved
...ges/komodo_defi_rpc_methods/lib/src/rpc_methods/withdrawal/send_raw_transaction_request.dart
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive support for the SIA protocol by implementing remaining fields and aligning the implementation with API documentation. The changes span multiple packages to fully integrate SIA-specific functionality for activation, withdrawals, and transaction handling.
Key changes:
- Adds SIA-specific fee handling with fallback logic for different field names (
total_amountvsamount) - Implements SIA protocol detection and legacy withdrawal flow routing
- Adds extensive logging, user action support, and improved activation polling logic for SIA
- Replaces SIA-specific withdrawal response types with transaction-agnostic structures
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/komodo_defi_types/lib/src/transactions/fee_info.dart | Adds SIA fee parsing with total_amount/amount fallback logic |
| packages/komodo_defi_sdk/lib/src/withdrawals/withdrawal_manager.dart | Adds SIA protocol detection and routes to legacy withdrawal API |
| packages/komodo_defi_sdk/lib/src/activation/protocol_strategies/sia_activation_strategy.dart | Enhances SIA activation with verbose logging, improved polling, and better error handling |
| packages/komodo_defi_rpc_methods/test/sia_rpc_methods_test.dart | Updates test to verify full SIA withdraw response structure |
| packages/komodo_defi_rpc_methods/lib/src/rpc_methods/withdrawal/withdrawal_rpc_namespace.dart | Removes SIA-specific convenience method and adds dedicated SIA raw transaction method |
| packages/komodo_defi_rpc_methods/lib/src/rpc_methods/withdrawal/sia_withdraw_request.dart | Deletes SIA-specific withdrawal request/response classes |
| packages/komodo_defi_rpc_methods/lib/src/rpc_methods/withdrawal/send_raw_transaction_request.dart | Adds SiaSendRawTransactionRequest class for SIA tx_json handling |
| packages/komodo_defi_rpc_methods/lib/src/rpc_methods/sia/sia_rpc_namespace.dart | Adds documentation and user action method for hardware wallet support |
| packages/komodo_defi_rpc_methods/lib/src/rpc_methods/sia/enable_sia.dart | Adds cancel response class, user action request, and improves documentation |
| packages/komodo_defi_rpc_methods/lib/src/rpc_methods/rpc_methods.dart | Removes export of deleted SIA withdraw request |
| packages/komodo_defi_rpc_methods/lib/src/common_structures/transaction_history/transaction_info.dart | Adds txJson field for SIA protocol transactions |
| packages/komodo_defi_rpc_methods/lib/src/common_structures/common_structures.dart | Removes export of empty/deleted file |
| packages/komodo_defi_rpc_methods/lib/src/common_structures/activation/strategies/utxo_activation_strategy.dart | Deletes empty file |
| packages/komodo_defi_rpc_methods/lib/src/common_structures/activation/activation_params/sia_activation_params.dart | Adds optional password field and improves documentation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
.../komodo_defi_rpc_methods/lib/src/common_structures/transaction_history/transaction_info.dart
Show resolved
Hide resolved
packages/komodo_defi_sdk/lib/src/activation/protocol_strategies/sia_activation_strategy.dart
Show resolved
Hide resolved
packages/komodo_defi_sdk/lib/src/activation/protocol_strategies/sia_activation_strategy.dart
Show resolved
Hide resolved
...ges/komodo_defi_rpc_methods/lib/src/rpc_methods/withdrawal/send_raw_transaction_request.dart
Show resolved
Hide resolved
...pc_methods/lib/src/common_structures/activation/activation_params/sia_activation_params.dart
Show resolved
Hide resolved
packages/komodo_defi_sdk/lib/src/activation/protocol_strategies/sia_activation_strategy.dart
Show resolved
Hide resolved
...pc_methods/lib/src/common_structures/activation/activation_params/sia_activation_params.dart
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/komodo_defi_sdk/lib/src/withdrawals/withdrawal_manager.dart (3)
499-501: Update comment to reflect Sia support.The comment on lines 499-500 mentions only Tendermint but the code now routes both Tendermint and Sia through the legacy implementation.
Apply this diff to update the comment:
- // Tendermint assets are not yet supported by the task-based API - // and require a legacy implementation + // Tendermint and Sia assets are not yet supported by the task-based API + // and require a legacy implementation if (isTendermintProtocol || isSiaProtocol) {
494-503: Extract protocol checking logic to reduce duplication.The protocol checking logic is duplicated across three methods (
previewWithdrawal,executeWithdrawal, andwithdraw). The TODO comment correctly identifies that this should be refactored into a strategy pattern or a getter on the protocol class.Consider extracting this into a helper method or adding a getter to the protocol classes:
bool _requiresLegacyWithdrawal(Asset asset) { return asset.protocol is TendermintProtocol || asset.protocol is SiaProtocol; }Then use it consistently:
if (_requiresLegacyWithdrawal(asset)) { return await _legacyManager.previewWithdrawal(parameters); }This centralizes the logic and makes it easier to add new legacy protocols.
708-710: Update comment to reflect Sia support.The comment should mention both Tendermint and Sia for consistency.
Apply this diff:
- // Tendermint assets are not yet supported by the task-based API - // and require a legacy implementation + // Tendermint and Sia assets are not yet supported by the task-based API + // and require a legacy implementation if (isTendermintProtocol || isSiaProtocol) {
🧹 Nitpick comments (4)
packages/komodo_defi_sdk/lib/src/activation/protocol_strategies/sia_activation_strategy.dart (3)
53-71: Verbose logging and init RPC usage are sound; consider reusing the RPC clientThe guarded verbose logging (behind
KdfLoggingConfig.verboseLogging) avoids unnecessaryjsonEncodework and gives good visibility into activation parameters. One minor improvement: you can constructfinal rpc = KomodoDefiRpcMethods(client);once and reuse it both here and in the polling loop to avoid repeated allocations.- final init = await KomodoDefiRpcMethods( - client, - ).sia.enableSiaInit(ticker: asset.id.id, params: params); + final rpc = KomodoDefiRpcMethods(client); + + final init = await rpc.sia.enableSiaInit( + ticker: asset.id.id, + params: params, + );
81-135: Polling loop logic is correct; consider adding timeout and richer progress infoThe
while (!isComplete)loop withstatus.isCompleted+Future.delayed(pollingInterval)is straightforward and should behave correctly for task-based activations.Two non-blocking improvements to consider:
- Add an optional timeout / max attempts guard to avoid endless polling in pathological cases (e.g., remote task never completes).
- Include
taskId(and possiblyasset.id.id) in the in‑progressadditionalInfomap so consumers can correlate updates without relying on external state.- var isComplete = false; - while (!isComplete) { + var isComplete = false; + // TODO(optional): support max duration/attempts to avoid infinite polling. + while (!isComplete) { @@ - yield ActivationProgress( - status: 'SIA activation in progress', - progressDetails: ActivationProgressDetails( - currentStep: ActivationStep.processing, - stepCount: 3, - additionalInfo: {'status': status.status}, - ), - ); + yield ActivationProgress( + status: 'SIA activation in progress', + progressDetails: ActivationProgressDetails( + currentStep: ActivationStep.processing, + stepCount: 3, + additionalInfo: { + 'taskId': taskId, + 'status': status.status, + }, + ), + );
137-150: Confirm intended behavior of yielding failure then rethrowingThe catch block both yields a failed
ActivationProgressand thenrethrows the exception. This means stream consumers may see a terminal “failed” progress event and also get an error on the stream.If callers already treat a thrown error as the authoritative signal, they might double-handle failure. Please confirm that downstream consumers expect both a final failed progress event and a stream error; otherwise, consider either omitting
rethrowor adding a comment documenting this contract.- } catch (e, s) { + } catch (e, s) { _log.severe('[SIA] Activation exception for ${asset.id.id}', e, s); @@ - ); - rethrow; + ); + // NOTE: We rethrow so callers can treat this as a hard error in addition + // to the final failed progress event. + rethrow;packages/komodo_defi_rpc_methods/lib/src/rpc_methods/withdrawal/send_raw_transaction_request.dart (1)
33-36: Redundant parse override.The
parsemethod simply delegates to the staticSendRawTransactionResponse.parse, which is already the default behavior. This override can be removed.Apply this diff to remove the redundant override:
- @override - SendRawTransactionResponse parse(Map<String, dynamic> json) => - SendRawTransactionResponse.parse(json);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
packages/komodo_defi_rpc_methods/lib/src/common_structures/activation/activation_params/sia_activation_params.dart(2 hunks)packages/komodo_defi_rpc_methods/lib/src/common_structures/activation/strategies/utxo_activation_strategy.dart(0 hunks)packages/komodo_defi_rpc_methods/lib/src/common_structures/common_structures.dart(0 hunks)packages/komodo_defi_rpc_methods/lib/src/common_structures/transaction_history/transaction_info.dart(4 hunks)packages/komodo_defi_rpc_methods/lib/src/rpc_methods/rpc_methods.dart(0 hunks)packages/komodo_defi_rpc_methods/lib/src/rpc_methods/sia/enable_sia.dart(3 hunks)packages/komodo_defi_rpc_methods/lib/src/rpc_methods/sia/sia_rpc_namespace.dart(3 hunks)packages/komodo_defi_rpc_methods/lib/src/rpc_methods/withdrawal/send_raw_transaction_request.dart(1 hunks)packages/komodo_defi_rpc_methods/lib/src/rpc_methods/withdrawal/sia_withdraw_request.dart(0 hunks)packages/komodo_defi_rpc_methods/lib/src/rpc_methods/withdrawal/withdrawal_rpc_namespace.dart(3 hunks)packages/komodo_defi_rpc_methods/test/sia_rpc_methods_test.dart(1 hunks)packages/komodo_defi_sdk/lib/src/activation/protocol_strategies/sia_activation_strategy.dart(5 hunks)packages/komodo_defi_sdk/lib/src/withdrawals/withdrawal_manager.dart(2 hunks)packages/komodo_defi_types/lib/src/transactions/fee_info.dart(1 hunks)
💤 Files with no reviewable changes (4)
- packages/komodo_defi_rpc_methods/lib/src/rpc_methods/rpc_methods.dart
- packages/komodo_defi_rpc_methods/lib/src/common_structures/common_structures.dart
- packages/komodo_defi_rpc_methods/lib/src/rpc_methods/withdrawal/sia_withdraw_request.dart
- packages/komodo_defi_rpc_methods/lib/src/common_structures/activation/strategies/utxo_activation_strategy.dart
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-05-26T21:42:19.450Z
Learnt from: takenagain
Repo: KomodoPlatform/komodo-defi-sdk-flutter PR: 63
File: packages/komodo_defi_types/lib/src/transactions/fee_info.dart:238-246
Timestamp: 2025-05-26T21:42:19.450Z
Learning: In KDF v2.4.0-beta, there's an asymmetric protocol for Tendermint fees: the backend returns "Tendermint" format in responses but expects "CosmosGas" format in requests from clients. The FeeInfo implementation correctly handles this by using fromJson() for "Tendermint" (backend responses) and toJson() for "CosmosGas" (client requests).
Applied to files:
packages/komodo_defi_rpc_methods/lib/src/common_structures/transaction_history/transaction_info.dartpackages/komodo_defi_types/lib/src/transactions/fee_info.dart
🔇 Additional comments (13)
packages/komodo_defi_sdk/lib/src/activation/protocol_strategies/sia_activation_strategy.dart (2)
2-18: Configurable polling interval and logger addition look goodThe added
pollingIntervalparameter/field and_loglogger integrate cleanly and keep defaults sensible (500ms). No issues spotted here.
43-49: Improved initial progress metadata is appropriateIncluding
assetTypeandprotocolinadditionalInfoon the initialActivationProgressis useful for consumers and keeps the event immutable viaconst. Looks good.packages/komodo_defi_rpc_methods/lib/src/common_structures/transaction_history/transaction_info.dart (1)
33-35: Good defensive null-safety pattern.The
containsKeycheck before parsingfee_detailsproperly handles missing fee information.packages/komodo_defi_rpc_methods/lib/src/rpc_methods/withdrawal/send_raw_transaction_request.dart (1)
38-64: LGTM: Well-structured SIA-specific request class.The
SiaSendRawTransactionRequestproperly encapsulates SIA protocol requirements with requiredtxJsonand clear documentation.packages/komodo_defi_rpc_methods/lib/src/rpc_methods/withdrawal/withdrawal_rpc_namespace.dart (2)
57-71: LGTM: Type update maintains consistency.The
JsonMap?type fortxJsonaligns with other SIA-related changes and provides better type safety.
73-85: LGTM: Clean SIA-specific convenience method.The
sendRawTransactionSiamethod provides a clear, typed interface for SIA withdrawals while keeping the legacy method for backward compatibility.packages/komodo_defi_rpc_methods/lib/src/common_structures/activation/activation_params/sia_activation_params.dart (1)
29-53: LGTM: Robust config parsing with good fallback logic.The
fromConfigJsonfactory properly handles bothserver_urlandnodes[].urlsources with clear error messages when neither is available.packages/komodo_defi_rpc_methods/lib/src/rpc_methods/sia/enable_sia.dart (3)
4-32: LGTM: Clear task initialization structure.The
TaskEnableSiaInitproperly nests activation params and provides clear documentation for the SIA activation flow.
88-126: LGTM: Well-designed user action handling.The
TaskEnableSiaUserActionclass properly encapsulates hardware wallet interactions (PIN/passphrase) with appropriate optional fields and clear serialization.
128-145: LGTM: Type-safe cancel response.The
SiaCancelResponseprovides better type safety than a generic response, making the API more self-documenting.packages/komodo_defi_rpc_methods/lib/src/rpc_methods/sia/sia_rpc_namespace.dart (1)
3-67: LGTM: Well-structured RPC namespace.The
SiaMethodsNamespaceprovides a clean, typed interface over the rawtask::enable_sia::*APIs with consistent parameter handling and clear documentation.packages/komodo_defi_rpc_methods/test/sia_rpc_methods_test.dart (2)
7-25: LGTM: Test validates nested activation params structure.The test properly verifies the nested
activation_params.client_confstructure and all required fields.
27-59: LGTM: Comprehensive SIA withdraw response validation.The test validates the complete response shape including
tx_json,fee_details, and all transaction fields, ensuring proper parsing of the SIA-specific structure.
...pc_methods/lib/src/common_structures/activation/activation_params/sia_activation_params.dart
Show resolved
Hide resolved
.../komodo_defi_rpc_methods/lib/src/common_structures/transaction_history/transaction_info.dart
Show resolved
Hide resolved
...ges/komodo_defi_rpc_methods/lib/src/rpc_methods/withdrawal/send_raw_transaction_request.dart
Show resolved
Hide resolved
packages/komodo_defi_sdk/lib/src/activation/protocol_strategies/sia_activation_strategy.dart
Show resolved
Hide resolved
thanks to @CharlVS 's patch file
standard withdraw models are being used regardless, with txJson and txHex added to WithdrawResult
other strategies use the completer approach with logging, which the sia strategy lacked
takenagain
force-pushed
the
refactor/add-remaining-sia-fields
branch
from
7289255 to
0d9160f
Compare
takenagain
force-pushed
the
refactor/add-remaining-sia-fields
branch
from
a1d6c9e to
473c371
Compare
A modified version of @CharlVS's patch file, excluding the withdrawal strategy refactor, to reduce the scope of changes for the time being (although it would be good to add at a later stage).
Summary by CodeRabbit
New Features
Improvements
Removals
✏️ Tip: You can customize this high-level summary in your review settings.