Harden library conversion foundation#32
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a ConversionTransactionBuilder in the SDK to automate CKB/iCKB conversion planning and a withdrawal_selection module for optimized ready-deposit selection. It implements "fail-closed" logic for blockchain scans reaching logical limits and adds validation for DAO output limits and deposit quantities. The PR also centralizes test helpers into a new testkit package. Review feedback identifies several improvement opportunities: providing scriptLenRange directly in API queries for pure-capacity cells, using clone() instead of from() for deep-copying transactions in loops to avoid shared state mutation, removing redundant manual filters, and refining DAO output-limit calculations to account for all planned transaction effects.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a "fail closed" policy for blockchain scans to ensure state completeness, supported by new utility helpers and a shared testkit package. Key functional enhancements include a robust conversion transaction builder in the SDK with retry logic for DAO output limits, a pool-friendly ready-deposit selector for withdrawals, and stricter validation for deposit quantities and order matching. Review feedback identified a logic error in maturity estimation for UDT-to-CKB conversions and raised concerns about potential usability bottlenecks caused by hardcoded scan limits in the public pool and account cell queries.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive conversion transaction builder in the SDK, supporting both CKB-to-iCKB and iCKB-to-CKB directions with integrated planning for direct deposits, withdrawals, and fallback orders. It implements a "fail-closed" scanning strategy across the stack using sentinel cells to ensure state completeness and adds a new @ickb/testkit package to unify test utilities. Feedback includes a warning regarding potential shared state mutation when using ccc.Transaction.from within loops and a suggestion to improve error message clarity in the errorOf helper.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request significantly enhances the iCKB stack by introducing a robust conversion transaction builder, pool-friendly withdrawal selection logic, and a "fail-closed" mechanism for cell scans to ensure state integrity. It also consolidates test utilities into a new centralized testkit and adds validation for DAO output limits and deposit quantities. The review feedback identifies several redundant manual filters that can be removed for better efficiency and points out a performance risk in the bitmask search algorithm, recommending the implementation of safety limits to prevent event loop blocking.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces significant enhancements to the iCKB SDK, including a new withdrawal selection mechanism, improved conversion transaction building, and robust L1 state scanning with sentinel-based safety checks. It also adds a new internal testkit package to standardize test fixtures. My review identified one area where BigInt comparison should be used instead of Number conversion to ensure precision and consistency.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new @ickb/testkit package for shared test helpers and refactors existing tests across the workspace to use it. It implements a "fail-closed" scan completeness boundary using collectCompleteScan to ensure incomplete blockchain state is not misinterpreted as zero balance. Key functional additions include a comprehensive buildConversionTransaction in the SDK for planning CKB/iCKB conversions, pool-friendly ready-deposit selection logic with singleton protection, and improved validation for deposit quantities and DAO output limits. Feedback focuses on optimizing transaction object handling to avoid redundant conversions, simplifying cell queries by removing redundant manual filters already covered by RPC parameters, and correcting the usage of CCC API query filters.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces significant enhancements to the iCKB SDK, including a robust conversion transaction builder, improved ready-withdrawal selection, and comprehensive scan-limit handling across various modules. Key changes include the addition of collectCompleteScan to ensure safe cell scanning, the introduction of DaoOutputLimitError for better error handling, and the creation of a @ickb/testkit package to centralize test fixtures. The SDK now supports planning direct deposits and withdrawal requests while respecting DAO output limits and chain tip consistency. I have reviewed the changes and found the feedback regarding the bot capacity scan in IckbSdk.getL1State to be actionable and necessary for consistency with the codebase's API usage patterns.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request enhances the iCKB stack by introducing a conversion transaction builder in the SDK, pool-friendly withdrawal selection logic, and 'fail-closed' cell scanning to ensure state completeness. It also refactors tests into a shared testkit and adds validation for deposit quantities and exchange ratios. Feedback identifies a logic error in the SDK's conversion builder where DAO transaction outputs are overestimated, which could lead to unnecessary transaction failures.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a 'Scan Completeness Boundary' to ensure data integrity by failing closed when cell scans reach logical limits. It adds a conversion transaction builder to the SDK for CKB/iCKB swaps, implements pool-friendly withdrawal selection, and centralizes test utilities into a new private testkit package. Refactoring was performed across multiple packages to adopt a unified collectCompleteScan pattern. Feedback suggests optimizing performance by moving lock script filtering from JavaScript to the RPC query level in several cell-finding implementations to reduce data transfer overhead.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces significant improvements to the iCKB SDK, including a new ready-withdrawal selection mechanism, enhanced conversion transaction planning, and improved error handling. It also adds a testkit package to standardize test fixtures across the workspace. The code changes include logic for direct deposit planning, withdrawal selection with anchor protection, and robust error reporting. I have reviewed the proposed errorMessage helper and agree that it should be updated to handle string errors and BigInt serialization more robustly to prevent precision loss and unhelpful output.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces significant enhancements to the iCKB SDK, including a new ready-withdrawal selection mechanism, a comprehensive conversion transaction builder, and improved scan completeness handling across the codebase. It also adds a @ickb/testkit package to centralize test fixtures and updates several existing tests to use these new utilities. The changes ensure that scans fail closed when limits are reached, improving reliability in state-dependent operations. My feedback highlights an inconsistency in getCkb where it uses a hardcoded limit, suggesting that it should be made configurable to align with the flexibility provided in getL1State.
| private async getCkb( | ||
| client: ccc.Client, | ||
| tip: ccc.ClientBlockHeader, | ||
| poolDeposits: PoolDepositState, | ||
| ): Promise<{ |
There was a problem hiding this comment.
The getCkb method uses a hardcoded defaultFindCellsLimit for scanning bot capacity cells and withdrawal groups. This is inconsistent with getL1State, which allows callers to customize other scan limits like poolDepositLimit and orderLimit. This could cause getL1State to fail for bots with a large number of cells, without providing a way for the caller to increase the relevant limits.
Consider passing configurable limits down from getL1State to getCkb and using them for the bot-related scans. For example, you could add botCapacityLimit and botWithdrawalLimit to GetL1StateOptions.
There was a problem hiding this comment.
Fixed in 31621c0698c67a18a67af8e6e110d125f56ae1e9. GetL1StateOptions now exposes botCapacityLimit and botWithdrawalLimit; getL1State(...) threads them into getCkb(...), preserving the same fail-closed complete-scan behavior with the caller-selected thresholds.
Added regression coverage for both custom bot capacity and bot withdrawal scan limits. Local checks passed before push: pnpm vitest run packages/sdk/src/sdk.test.ts, pnpm --filter @ickb/sdk lint, and full pnpm check.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a robust conversion transaction builder in the SDK, supporting both CKB-to-iCKB and iCKB-to-CKB directions with sophisticated planning policies, including pool-friendly ready-deposit selection and dust order handling. A significant architectural improvement is the implementation of a "fail-closed" sentinel scanning pattern across all packages to ensure that incomplete blockchain state scans are treated as errors rather than zero balances. Additionally, the PR adds a new @ickb/testkit package for shared test utilities, refactors existing tests for better maintainability, and enhances validation for DAO output limits and exchange ratios. I have no feedback to provide.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a robust "fail-closed" mechanism for cell scanning across the stack, ensuring that incomplete state is treated as an error rather than a zero balance. It adds a new @ickb/testkit package to centralize test helpers and refactors existing tests to use it. Key functional additions include a comprehensive buildConversionTransaction helper in the SDK for planning CKB/iCKB conversions, improved withdrawal selection logic with singleton protection, and enhanced validation for DAO output limits and deposit quantities. Documentation has been updated to reflect these changes, particularly regarding scan completeness boundaries and conversion planning. I have no feedback to provide as no review comments were submitted.
|
LGTM Phroi %285 |
1 participant
Why
Changes
@ickb/testkithelpers and package test wiring.