Harden library transaction and protocol boundaries#29
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the iCKB monorepo to centralize transaction completion logic within the SDK, introduces specific DAO cell types for improved type safety, and enhances order resolution and state management. Key additions include completeIckbTransaction and buildBaseTransaction in the SDK, alongside a new selectBoundedUdtSubset utility. Feedback identifies a critical performance and logic risk in selectBoundedUdtSubset due to an excessively high bitmask search limit. Furthermore, the reviewer recommends optimizing findOrigin and findWithdrawalGroups by implementing caching and parallelizing RPC calls to reduce network latency during cell discovery.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the iCKB SDK and core libraries to streamline transaction construction and completion, introducing utilities like completeIckbTransaction and projectAccountAvailability. It also addresses limit order ambiguity by failing closed on equal-score descendants and improves exchange ratio precision using BigInt comparisons. Feedback suggests refining the assertCompleteScan logic to avoid blocking users who have exactly the limit number of cells, as the current check may trigger a false positive for incomplete state.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the iCKB Stack to centralize transaction completion logic and improve concurrency in RPC fetching. Key changes include the introduction of specific DAO cell types, a refined order resolution heuristic, and enhanced account state projection. Feedback identifies opportunities to optimize performance by deduplicating redundant RPC calls in scanning loops and addressing potential memory pressure in the subset enumeration logic. Additionally, it is recommended to centralize the compareBigInt utility to reduce code duplication across the codebase.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request optimizes the iCKB stack by parallelizing RPC requests and implementing caching for cell and header fetching across the core, DAO, and order packages. It also refines the SDK's transaction completion workflow and account state management. Review feedback highlights opportunities to further reduce redundant RPC calls by sharing caches in OwnedOwnerManager and recording transaction responses. Other suggestions include refining the assertCompleteScan logic to prevent failures at exact limits, optimizing the subset selection algorithm in utils, and reducing latency in the transaction confirmation polling loop.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the iCKB stack to improve performance through concurrent header fetching and caching, while enhancing type safety by introducing specific DAO cell types. It also updates the SDK to centralize transaction completion logic and adds robust confirmation handling. Feedback identified a critical bug in the order matching loop condition that prevents execution and a logic error in the sorting of order matchers that could result in sub-optimal trade selection.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request hardens the iCKB stack by implementing concurrent and cached header fetching, improving order resolution robustness, and fixing bugs in fee calculation and heap management. It also enhances the SDK with new utilities for account state projection, transaction completion, and confirmation. Feedback emphasizes the importance of using bigint arithmetic for large value calculations to prevent precision loss.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the iCKB Stack to improve transaction completion, concurrency, and account state management. Key changes include introducing a shared completion path in @ickb/sdk, refactoring DAO cell types for better specificity, and implementing concurrent header fetching with caching across core managers. The update also enhances the order matching logic with partial caps and ambiguity handling, adds utility functions for UDT subset selection, and includes comprehensive test coverage for new features like account availability projection and transaction sending. Review feedback correctly identified and fixed critical issues in DataView decoding for subarrays and a conversion direction error in UDT-to-CKB fee calculations.
|
LGTM Phroi %322 |
Why
Stack libraries are still unpublished, so the library surface should enforce current protocol and transaction invariants directly instead of carrying compatibility seams or caller-side rituals. This PR tightens the foundation before the app and bot runtime migrations land on top.
Changes
Validation
Notes