Skip to content

fix(zhtlc): align orderbook RPC with KDF schema, and add id pagination support#229

Merged
takenagain merged 6 commits intobugfix/zhltc-activation-fixesfrom
bugfix/orderbook-rpc-schema
Sep 28, 2025
Merged

fix(zhtlc): align orderbook RPC with KDF schema, and add id pagination support#229
takenagain merged 6 commits intobugfix/zhltc-activation-fixesfrom
bugfix/orderbook-rpc-schema

Conversation

@takenagain
Copy link
Copy Markdown
Contributor

@takenagain takenagain commented Sep 28, 2025

Summary by CodeRabbit

  • New Features
    • Enhanced orderbook parsing to support structured prices, volumes (including aggregated), addresses, and confirmation settings.
    • Added paging metadata to transaction history responses, supporting both page-number and from-id modes.
    • Broadened ZHTLC transaction history to handle multiple pagination types.
  • Refactor
    • Strategy factory now accepts optional custom strategies, with improved selection logic.
    • More robust JSON handling for numeric and string conversions.
  • Style
    • Adjusted loading text color in the withdraw source address field for better theme consistency.
  • Tests
    • Added comprehensive fixtures and tests for orderbook parsing and strategy selection.

@takenagain takenagain self-assigned this Sep 28, 2025
@takenagain takenagain added the bug Something isn't working label Sep 28, 2025
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Sep 28, 2025

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

Introduces new orderbook structures (OrderAddress, updated OrderInfo) with nested numeric parsing, adds NumericValue type, enhances pagination deserialization and propagates paging options through transaction history responses and strategies (including ZHTLC support for transaction-based pagination). Updates JSON utils for int→String conversion, adds tests/fixtures, and tweaks a UI text color.

Changes

Cohort / File(s) Summary
Orderbook structures
packages/komodo_defi_rpc_methods/lib/src/common_structures/orderbook/order_address.dart, .../order_info.dart, .../primitive/numeric_value.dart
Adds OrderAddress and OrderAddressType; rewrites OrderInfo to v2 schema with nested NumericValue and optional fields; implements NumericValue with decimal/fraction/rational parsing, tryParse, and JSON (de)serialization.
Pagination and JSON utils
packages/komodo_defi_rpc_methods/lib/src/common_structures/pagination/pagination.dart, packages/komodo_defi_types/lib/src/utils/json_type_utils.dart
Adds Pagination.fromJson supporting multiple key variants and type coercions; updates JSON traversal to convert int to String for String targets.
Transaction history strategies and response
packages/komodo_defi_rpc_methods/lib/src/rpc_methods/transaction_history/my_tx_history.dart, packages/komodo_defi_sdk/lib/src/transaction_history/strategies/etherscan_transaction_history_strategy.dart, .../zhtlc_transaction_strategy.dart, .../transaction_history_strategies.dart, packages/komodo_defi_types/lib/src/transactions/transaction_history_strategy.dart
MyTxHistoryResponse gains pagingOptions; Etherscan strategy populates pagingOptions; ZHTLC strategy supports transaction-based pagination and refactors request param handling; factory allows injecting strategies; removes a legacy pagination helper in types.
Tests and fixtures
packages/komodo_defi_rpc_methods/test/fixtures/orderbook/orderbook_response.json, .../test/src/common_structures/orderbook/order_info_test.dart, packages/komodo_defi_sdk/test/transaction_history/transaction_history_strategies_test.dart
Adds comprehensive orderbook fixture; tests OrderInfo parsing/round-trip; tests strategy selection prioritizing ZHTLC and order independence.
UI tweak
packages/komodo_ui/lib/src/defi/withdraw/source_address_field.dart
Changes loading text color source from colorScheme.onSurface to textTheme.bodyMedium?.color with same alpha application.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor UI
  participant Factory as TransactionHistoryStrategyFactory
  participant Strategy as Strategy (V2/Legacy/Etherscan/ZHTLC)
  participant RPC as RPC / Provider

  UI->>Factory: forAsset(asset, auth, pubkeyManager)
  Factory-->>UI: Strategy instance

  UI->>Strategy: fetchTransactionHistory(pagination, limit)
  alt Page-based
    Strategy->>RPC: getHistory(limit, page=pageNumber)
    RPC-->>Strategy: txs
    Strategy-->>UI: MyTxHistoryResponse(txs, pagingOptions: {page_number})
  else Transaction-based
    Strategy->>RPC: getHistory(limit, from_id)
    RPC-->>Strategy: txs
    Strategy-->>UI: MyTxHistoryResponse(txs, pagingOptions: {from_id})
  else Unsupported
    Strategy-->>UI: throw UnsupportedError
  end
Loading
sequenceDiagram
  autonumber
  participant Net as RPC Orderbook JSON
  participant Parser as OrderInfo.fromJson
  participant Sub as NumericValue / OrderAddress / ConfSettings

  Net-->>Parser: order entry JSON (v2 schema)
  Parser->>Sub: parse price/base_*/rel_* as NumericValue
  Parser->>Sub: parse address as OrderAddress(address_type, address_data)
  Parser->>Sub: parse conf_settings
  Sub-->>Parser: typed objects
  Parser-->>Caller: OrderInfo(fields nullable, nested values)
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

Suggested reviewers

  • smk762
  • CharlVS

Poem

I nibbled the bytes and parsed the grain,
New orders bloom on the data plain.
Pages turn with a gentle hop,
From IDs to numbers—no need to stop.
With whiskered wit and structured cheer,
I ship these changes—spring is here! 🐇✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title accurately captures the primary objectives of the pull request by stating the alignment of the orderbook RPC with the KDF schema and the addition of ID-based pagination support, and it uses a clear conventional prefix without extraneous detail. It is concise, specific to the main changes described in the diff summaries, and would be immediately understandable to team members reviewing the commit history.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Sep 28, 2025

Visit the preview URL for this PR (updated for commit 0b84aab):

https://kdf-sdk--pr229-bugfix-orderbook-rpc-1ins2quq.web.app

(expires Sun, 05 Oct 2025 22:53:40 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: 9c1b6e6c010cf0b965c455ba7a69c4aedafa8a1d

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Sep 28, 2025

Visit the preview URL for this PR (updated for commit 0b84aab):

https://komodo-playground--pr229-bugfix-orderbook-rpc-3w3w1ml8.web.app

(expires Sun, 05 Oct 2025 22:53:25 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: 2bfedd77fdea45b25ba7c784416e81f177aa5c47

@takenagain
Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@takenagain takenagain marked this pull request as ready for review September 28, 2025 22:22
Copilot AI review requested due to automatic review settings September 28, 2025 22:22
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Sep 28, 2025

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link
Copy Markdown
Contributor

Copilot AI left a 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 aligns the ZHTLC orderbook RPC implementation with the KDF schema and adds support for ID-based pagination. The changes include:

  • Enhanced pagination support for ZHTLC transactions with ID-based pagination
  • Updated orderbook schema to match KDF format with structured address and numeric value types
  • Improved JSON type conversion utilities and added comprehensive test coverage

Reviewed Changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.

Show a summary per file
File Description
source_address_field.dart UI color theme fix for text styling
json_type_utils.dart Added safe int-to-string conversion for JSON parsing
transaction_history_strategy.dart Removed unused legacy pagination helper method
transaction_history_strategies_test.dart Added comprehensive test coverage for ZHTLC strategy selection
transaction_history_strategies.dart Added configurable strategy injection and improved error message formatting
zhtlc_transaction_strategy.dart Enhanced pagination support with ID-based pagination for ZHTLC
etherscan_transaction_history_strategy.dart Added pagination options field to response structure
order_info_test.dart Added comprehensive tests for orderbook schema parsing
orderbook_response.json Added test fixture for orderbook API responses
my_tx_history.dart Added pagination options field to transaction history response
numeric_value.dart Implemented complete numeric value structure for KDF schema alignment
pagination.dart Added JSON parsing support for pagination parameters
order_info.dart Complete rewrite to match KDF orderbook schema
order_address.dart Added structured address type for orderbook entries

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
packages/komodo_defi_sdk/lib/src/transaction_history/transaction_history_strategies.dart (1)

13-20: Freeze the strategy list to avoid aliasing surprises.

Right now _strategies holds a directly passed-in list, so anyone keeping that reference can mutate the factory’s internals after construction. Wrapping the source in List.unmodifiable(...) would lock the roster down while still allowing custom injection.

Consider:

-  }) : _strategies =
-           strategies ??
-           [
-             EtherscanTransactionStrategy(pubkeyManager: pubkeyManager),
-             V2TransactionStrategy(auth),
-             const LegacyTransactionStrategy(),
-             const ZhtlcTransactionStrategy(),
-           ];
+  }) : _strategies = List.unmodifiable(
+          strategies ??
+              [
+                EtherscanTransactionStrategy(pubkeyManager: pubkeyManager),
+                V2TransactionStrategy(auth),
+                const LegacyTransactionStrategy(),
+                const ZhtlcTransactionStrategy(),
+              ],
+        );
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8626160 and a353d06.

📒 Files selected for processing (14)
  • packages/komodo_defi_rpc_methods/lib/src/common_structures/orderbook/order_address.dart (1 hunks)
  • packages/komodo_defi_rpc_methods/lib/src/common_structures/orderbook/order_info.dart (1 hunks)
  • packages/komodo_defi_rpc_methods/lib/src/common_structures/pagination/pagination.dart (1 hunks)
  • packages/komodo_defi_rpc_methods/lib/src/common_structures/primitive/numeric_value.dart (1 hunks)
  • packages/komodo_defi_rpc_methods/lib/src/rpc_methods/transaction_history/my_tx_history.dart (5 hunks)
  • packages/komodo_defi_rpc_methods/test/fixtures/orderbook/orderbook_response.json (1 hunks)
  • packages/komodo_defi_rpc_methods/test/src/common_structures/orderbook/order_info_test.dart (1 hunks)
  • packages/komodo_defi_sdk/lib/src/transaction_history/strategies/etherscan_transaction_history_strategy.dart (1 hunks)
  • packages/komodo_defi_sdk/lib/src/transaction_history/strategies/zhtlc_transaction_strategy.dart (2 hunks)
  • packages/komodo_defi_sdk/lib/src/transaction_history/transaction_history_strategies.dart (6 hunks)
  • packages/komodo_defi_sdk/test/transaction_history/transaction_history_strategies_test.dart (1 hunks)
  • packages/komodo_defi_types/lib/src/transactions/transaction_history_strategy.dart (0 hunks)
  • packages/komodo_defi_types/lib/src/utils/json_type_utils.dart (1 hunks)
  • packages/komodo_ui/lib/src/defi/withdraw/source_address_field.dart (1 hunks)
💤 Files with no reviewable changes (1)
  • packages/komodo_defi_types/lib/src/transactions/transaction_history_strategy.dart
🔇 Additional comments (4)
packages/komodo_defi_types/lib/src/utils/json_type_utils.dart (1)

129-134: Solid int-to-string handling

Thanks for adding the exact int→String path; it removes the need for lossy casting while keeping the behavior deterministic.

packages/komodo_defi_sdk/lib/src/transaction_history/strategies/etherscan_transaction_history_strategy.dart (1)

113-117: Paging metadata mirrors incoming request cleanly.

Good call echoing the active pagination mode back through pagingOptions; it keeps the response aligned with the new schema without disturbing the existing sorting/paging flow.

packages/komodo_defi_sdk/lib/src/transaction_history/strategies/zhtlc_transaction_strategy.dart (1)

21-34: Unified pagination handling reads well.

The record-based switch keeps both page- and id-based flows compact and makes the downstream RPC call much clearer.

packages/komodo_defi_rpc_methods/lib/src/common_structures/orderbook/order_info.dart (1)

125-139: Confirm SDK constraint for null-aware map entries.

These map literals rely on the Dart 3.8+ null-aware element syntax ('key': ?value). Please double-check that the package’s environment.sdk constraint (and any package-level language version overrides) already guarantee Dart ≥3.8 so this compiles everywhere we publish or consume it. If not, we should bump the constraint in the relevant pubspecs before merging.

@takenagain takenagain merged commit ec445d3 into bugfix/zhltc-activation-fixes Sep 28, 2025
2 of 3 checks passed
@takenagain takenagain deleted the bugfix/orderbook-rpc-schema branch September 28, 2025 22:49
CharlVS added a commit that referenced this pull request Oct 2, 2025
* Enhance ZHTLC activation with improved config parsing and RPC methods

Co-authored-by: charl <charl@vanstaden.info>

* feat(zhtlc): integrate ZHTLC types and task RPCs per KDF API

* dragon_logs: remove tracked ignored files and add .gitignore files

* Integrate and verify ZHTLC in komodo_defi_sdk (#196)

* Refactor ZHTLC activation strategy with stream-based task handling

Co-authored-by: charl <charl@vanstaden.info>

* Refactor activation steps to use strongly-typed ActivationStep enum

Co-authored-by: charl <charl@vanstaden.info>

---------

Co-authored-by: Cursor Agent <cursoragent@cursor.com>

* chore: run code generators

* chore(git): delete previously tracked igored files

* Fix ERC20 and ZHTLC activation params serialization and method calls (#212)

Co-authored-by: Cursor Agent <cursoragent@cursor.com>

* fix: misc fixes

* docs(activation): add activation params refactoring plan

Clean base/subclass separation, canonical serialization + encoder strategy, user configuration + persistence, BLoC timeout, SDK integration via AssetId extension and resolveActivationParams, and migration steps. No backwards compatibility required.

* docs(sdk): activation refactoring v2

* feat(activation,zhtlc,sdk,rpc)!: extract ZHTLC fields to ZhtlcActivationParams; add ActivationConfigService; unify priv_key_policy serialization; normalize toRpcParams; formatting fixes

* Checkpoint before follow-up message

Co-authored-by: charl <charl@vanstaden.info>

* Fix: Remove unused import statement

Co-authored-by: charl <charl@vanstaden.info>

* feat: Add support for electrum_servers in Zhtlc activation

Co-authored-by: charl <charl@vanstaden.info>

* feat: initialize Firebase hosting for SDK example and playground

- Add Firebase hosting configuration for packages/komodo_defi_sdk/example
  - Configure to use kdf-sdk hosting site on komodo-defi-sdk project
  - Set up hosting target mapping
- Add Firebase hosting configuration for playground
  - Configure to use default hosting site on komodo-playground project
- Update GitHub Actions workflows to use target parameter for SDK example
- Update .gitignore to allow Firebase config files in example and playground

* fix: update z-coin activation strategy and example UI

- Update zhtlc_activation_strategy for improved activation handling
- Add activation config service enhancements
- Update logged_in_view_widget with improved UI for instance management
- Export additional SDK functionality in public API

* docs(firebase): reorganize Firebase deployment scripts and documentation

- Move Firebase scripts to .github/scripts/firebase/ directory
- Rename scripts for clarity:
  - setup-firebase-secrets.sh → setup-github-secrets.sh
  - verify-firebase-secrets.sh → verify-github-secrets.sh
- Move documentation to docs/firebase/ directory
- Rename documentation file to firebase-deployment-setup.md
- Update script paths in documentation
- Add README for scripts directory

* fix: remove global singleton registration of ActivationConfigService

- Remove global GetIt.I registration to prevent conflicts between multiple SDK instances
- Pass ActivationConfigService through constructor chain instead of global resolution
- Update ZhtlcActivationStrategy to accept service via constructor
- Update ActivationStrategyFactory and ActivationManager to propagate the service
- Prevent memory leaks and use-after-dispose issues when SDK instances are disposed

This ensures each SDK instance maintains its own isolated ActivationConfigService,
fixing issues where only the first SDK's service would be globally registered.

* fix(activation): register ActivationConfigService during bootstrap

- Ensure service is available before ActivationManager to fix ZHTLC/Z-coin activation flow\n- Remove duplicate registration from KomodoDefiSdk.init\n- Example: use instance sdk instead of context.read for ZHTLC prompt

* docs(tech-debt): add Activation & ZHTLC tech-debt report and refine sections

* feat(komodo_defi_sdk): add HiveActivationConfigRepository and integrate in activation flow; update bootstrap, activation service, and example widget

* fix: resolve external logging callback flaws and memory leak

- Fix native callback memory leak by disposing _kdfOperations in KomodoDefiFramework
- Remove debug-only conditional logging in bootstrap.dart
- Fix potential double subscription in komodo_defi_sdk.dart
- Add comprehensive error handling to log stream listeners
- Replace empty catch blocks with proper error handling
- Add safe logging helpers to prevent callback exceptions from crashing
- Ensure log streams continue operating even after callback errors

* refactor(withdrawals): inject LegacyWithdrawalManager and use for Tendermint flows

- add _legacyManager constructor param and field
- delegate Tendermint preview/withdraw to injected manager
- minor formatting and watch() simplifications

* fix(kdf-ops): use conditional imports for platform-specific imports

i.e. dart:ffi

* fix(activation-params): include other RPC server keys

* fix(activation-config-repository): register adapter type

* feat(zhtlc): add desktop zcash params downloader (#228)

* feat(sdk): add zhltc params file downloader

* feat(sdk-example): integrate zcash downloader for desktop

* chore: remove groth from default params download list

* test(komodo-defi-sdk): fix failing zcash unit tests

* refactor: clean up imports, tests, and dialog date conversion

* feat(zhtlc): add mobile config params downloader

* feat(zhtlc): web lightwallet server URLs workaround

* fix(zhtlc): align orderbook RPC with KDF schema, and add id pagination support (#229)

* feat(zhtlc-tx): add from_id pagination support from RPC docs

* fix(orderbook-v2): use v2 fields and types

* fix(source-address-field): use consistent text theme to fix visibility

* fix(order-address): change address data to optional for shielded ops

* fix(json-utils): convert int to string in traverse as it's lossless

* test(sdk): fix missing type in zcash mock classes

* fix(address-select-input): use consistent text theme to fix visibility

* feat(zhtlc): estimate activation percentage based on block height (#230)

* feat(zhtlc): estimate activation percentage based on block height

* refactor(review): remove unnecessary casts, states, and configurable polling

* fix(config-transform): only replace lightwalletservers if wss is not empty

* Fix ZHTLC activation issues and improve resilience

Co-authored-by: charl <charl@vanstaden.info>

---------

Co-authored-by: Charl (Nitride) <77973576+CharlVS@users.noreply.github.com>
Co-authored-by: Cursor Agent <cursoragent@cursor.com>
Co-authored-by: charl <charl@vanstaden.info>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants