Adopt core protocol libraries#60
Conversation
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
|
/gemini review |
![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 major refactoring across the @ickb monorepo packages, including core, dao, order, and sdk. Key updates include transitioning relative imports to .ts extensions, adding api-extractor configurations, updating package scripts, and introducing comprehensive JSDoc documentation. Additionally, the IckbUdt class has been refactored to extend ccc.Udt with custom completion logic, and Nervos DAO output limit checks have been streamlined, removing redundant mocks and assertions from the test suites. There are no review comments to address, and the changes appear to be a clean and thorough modernization of the codebase.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
CCC cleanup note: this branch now removes the custom local CCC fork workflow ( |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the workspace to simplify dependencies by resolving CCC packages directly through the lockfile, removing the local CCC fork build step, and updating import paths to use .ts extensions. It restructures and modularizes the core, dao, and order packages, introducing API Extractor configurations, robust transaction-shaping helpers, and comprehensive test suites. The review feedback highlights several opportunities to improve debuggability by replacing vague error messages with descriptive, context-rich errors that include transaction hashes, block numbers, or outpoints. Additionally, a critical issue was identified in packages/core/src/udt.ts where a missing input cell could lead to a runtime crash during transaction completion.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| const cells = await Promise.all( | ||
| tx.inputs.map(async (input) => input.getCell(client)), | ||
| ); |
There was a problem hiding this comment.
The input.getCell(client) call can return undefined if the input cell is not found on-chain or in the indexer. Passing undefined directly to this.inputContribution will cause a runtime crash (TypeError) inside ccc.CellAny.from(cellLike). Consider validating that the cell is defined and throwing a descriptive error if it is not.
const cells = await Promise.all(
tx.inputs.map(async (input, index) => {
const cell = await input.getCell(client);
if (cell === undefined) {
throw new Error(`Input cell not found for out point: ${input.previousOutput.txHash}#${input.previousOutput.index}`);
}
return cell;
}),
);There was a problem hiding this comment.
Handled at the input-tally boundary: CellInput.getCell collapses missing cells to Unable to complete input, so this now wraps that specific error with Input cell not found: <previousOutput> and leaves other client errors unchanged. Added a regression in packages/core/test/udt/udt_complete.ts.
| transactionCache, | ||
| ))?.header; | ||
| if (header === undefined) { | ||
| throw new Error("Header not found for txHash"); |
There was a problem hiding this comment.
The error message is vague and does not include the actual transaction hash (txHash), which makes debugging difficult. Consider including the transaction hash in the error message.
| throw new Error("Header not found for txHash"); | |
| throw new Error(`Header not found for transaction: ${cell.outPoint.txHash}`); |
References
- Replace vague error guards with explicit checks and descriptive messages for distinct failure modes in transaction building to improve user feedback and debuggability.
There was a problem hiding this comment.
Updated this missing-header error to include the transaction hash for the protocol input.
| const txWithHeader = await txWithHeaderPromise; | ||
| if (!txWithHeader?.header) { | ||
| if (txWithHeader?.header === undefined) { | ||
| throw new Error("Header not found for txHash"); |
There was a problem hiding this comment.
The error message is vague and does not include the actual transaction hash (txHash), which makes debugging difficult. Consider including the transaction hash in the error message.
| throw new Error("Header not found for txHash"); | |
| throw new Error(`Header not found for transaction: ${txHash}`); |
References
- Replace vague error guards with explicit checks and descriptive messages for distinct failure modes in transaction building to improve user feedback and debuggability.
There was a problem hiding this comment.
Updated the receipt-cell missing-header error to include the transaction hash.
| const txHash = cell.outPoint.txHash; | ||
| const txWithHeader = await getCachedTransactionWithHeader(options, txHash); | ||
| if (txWithHeader?.header === undefined) { | ||
| throw new Error("Header not found for txHash"); |
There was a problem hiding this comment.
The error message is vague and does not include the actual transaction hash (txHash), which makes debugging difficult. Consider including the transaction hash in the error message.
| throw new Error("Header not found for txHash"); | |
| throw new Error(`Header not found for transaction: ${txHash}`); |
References
- Replace vague error guards with explicit checks and descriptive messages for distinct failure modes in transaction building to improve user feedback and debuggability.
There was a problem hiding this comment.
Updated the DAO deposit missing-header error to include the transaction hash.
| getCachedTransactionWithHeader(options, txHash), | ||
| ]); | ||
| if (depositHeader === undefined) { | ||
| throw new Error("Header not found for block number"); |
There was a problem hiding this comment.
The error message is vague and does not include the actual block number (depositBlockNumber), which makes debugging difficult. Consider including the block number in the error message.
| throw new Error("Header not found for block number"); | |
| throw new Error(`Header not found for block number: ${depositBlockNumber}`); |
References
- Replace vague error guards with explicit checks and descriptive messages for distinct failure modes in transaction building to improve user feedback and debuggability.
There was a problem hiding this comment.
Updated the DAO withdrawal deposit-header error to include the missing deposit block number.
| throw new Error("Header not found for block number"); | ||
| } | ||
| if (txWithHeader?.header === undefined) { | ||
| throw new Error("Header not found for txHash"); |
There was a problem hiding this comment.
The error message is vague and does not include the actual transaction hash (txHash), which makes debugging difficult. Consider including the transaction hash in the error message.
| throw new Error("Header not found for txHash"); | |
| throw new Error(`Header not found for transaction: ${txHash}`); |
References
- Replace vague error guards with explicit checks and descriptive messages for distinct failure modes in transaction building to improve user feedback and debuggability.
There was a problem hiding this comment.
Updated the DAO withdrawal transaction-header error to include the transaction hash.
| const cell = | ||
| cellLike instanceof ccc.OutPoint ? await client.getCell(cellLike) : cellLike; | ||
| if (cell === undefined) { | ||
| throw new Error("Cell not found"); |
There was a problem hiding this comment.
The error message is vague and does not include the outpoint if cellLike is an OutPoint, which makes debugging difficult. Consider including the outpoint details in the error message.
const outPointStr = cellLike instanceof ccc.OutPoint ? `${cellLike.txHash}#${cellLike.index}` : "provided cell";
throw new Error(`Cell not found: ${outPointStr}`);References
- Replace vague error guards with explicit checks and descriptive messages for distinct failure modes in transaction building to improve user feedback and debuggability.
There was a problem hiding this comment.
Updated DAO deposit outpoint lookups to report the missing outpoint with toHex().
| cellLike instanceof ccc.OutPoint ? await client.getCell(cellLike) : cellLike; | ||
| if (cell === undefined) { | ||
| throw new Error("Cell not found"); | ||
| } |
There was a problem hiding this comment.
The error message is vague and does not include the outpoint if cellLike is an OutPoint, which makes debugging difficult. Consider including the outpoint details in the error message.
| } | |
| const outPointStr = cellLike instanceof ccc.OutPoint ? `${cellLike.txHash}#${cellLike.index}` : "provided cell"; | |
| throw new Error(`Cell not found: ${outPointStr}`); |
References
- Replace vague error guards with explicit checks and descriptive messages for distinct failure modes in transaction building to improve user feedback and debuggability.
There was a problem hiding this comment.
Updated DAO withdrawal outpoint lookups to report the missing outpoint with toHex().
|
/gemini review |
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request simplifies the workspace by removing the local CCC fork build workflow and resolving CCC packages as standard package dependencies. It also refactors and reorganizes the core, dao, and order packages, integrating api-extractor and updating build scripts. Additionally, the matching logic in the order package has been modularized, and IckbUdt in the core package has been refactored to handle transaction completion directly. Feedback on the changes includes a recommendation to use a TypeScript type guard in order_match_sequence.ts to properly narrow the type of filtered order matchers.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| return orderPool | ||
| .map((o) => OrderMatcher.from(o, isCkb2Udt, ckbMiningFee)) | ||
| .filter((m) => m !== undefined) | ||
| .sort((a, b) => OrderMatcher.compareRealRatioDesc(a, b)); |
There was a problem hiding this comment.
In strict TypeScript mode, filtering an array of union types (like (OrderMatcher | undefined)[]) using .filter(m => m !== undefined) does not automatically narrow the type to OrderMatcher[]. To ensure type safety and avoid compilation errors under strict null checks, use a type guard function in the .filter call.
| return orderPool | |
| .map((o) => OrderMatcher.from(o, isCkb2Udt, ckbMiningFee)) | |
| .filter((m) => m !== undefined) | |
| .sort((a, b) => OrderMatcher.compareRealRatioDesc(a, b)); | |
| return orderPool | |
| .map((o) => OrderMatcher.from(o, isCkb2Udt, ckbMiningFee)) | |
| .filter((m): m is OrderMatcher => m !== undefined) | |
| .sort((a, b) => OrderMatcher.compareRealRatioDesc(a, b)); |
There was a problem hiding this comment.
Made the matcher filter an explicit type guard: .filter((m): m is OrderMatcher => m !== undefined). Local pnpm check and the GitHub build check are green on the pushed head.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request simplifies the workspace setup by resolving CCC packages as normal package dependencies, removing the need for a local CCC fork build step. It also refactors the core, dao, and order packages by migrating relative imports to .ts extensions, adding comprehensive JSDoc documentation, extracting internal helper functions, and reorganizing test suites into dedicated test directories. Feedback on the changes suggests avoiding optional chaining when retrieving elements from internal collections using a previously validated index in packages/core/src/owned_owner.ts, recommending throwing an explicit error instead to make invariant breaks visible.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| function assertWithdrawalRequestOutput( | ||
| withdrawalOutput: ccc.CellOutput | undefined, | ||
| ownerScript: ccc.Script, | ||
| daoScript: ccc.Script, | ||
| ): void { | ||
| if ( | ||
| withdrawalOutput?.lock.eq(ownerScript) !== true || | ||
| withdrawalOutput.type?.eq(daoScript) !== true | ||
| ) { | ||
| throw new Error("DAO withdrawal request output order changed"); | ||
| } | ||
| } |
There was a problem hiding this comment.
When retrieving elements from internal collections using a previously validated index, avoid optional chaining (?.). If the element is missing, throw an explicit error to make invariant breaks visible rather than silently skipping or returning undefined.
function assertWithdrawalRequestOutput(
withdrawalOutput: ccc.CellOutput | undefined,
ownerScript: ccc.Script,
daoScript: ccc.Script,
): void {
if (withdrawalOutput === undefined) {
throw new Error("Withdrawal request output is missing");
}
if (
withdrawalOutput.lock.eq(ownerScript) !== true ||
withdrawalOutput.type?.eq(daoScript) !== true
) {
throw new Error("DAO withdrawal request output order changed");
}
}References
- When retrieving elements from internal collections using a previously validated index, avoid optional chaining (?.). If the element is missing, throw an explicit error to make invariant breaks visible rather than silently skipping or returning undefined.
There was a problem hiding this comment.
Made the invariant explicit: assertWithdrawalRequestOutput now throws DAO withdrawal request output is missing before checking script order, and the lock comparison no longer uses optional chaining. Local pnpm check and GitHub build are green on the pushed head.
|
/gemini review |
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
|
/gemini review |
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
1 participant
Why
Continue the migration after the shared utility packages by moving the core protocol surfaces into their package-ready shape before the app, bot, SDK, and validation slices.
This also removes the custom local CCC fork workflow so the stack builds and tests against mainline published
@ckb-ccc/*packages from the workspace catalog and lockfile.Changes
@ickb/core,@ickb/dao, and@ickb/orderaround explicit package entry points, build configs, API Extractor configs, and package-local tests.forks:ccc, localforks/ccc/repo, fork smoke/override checks, and deploy/supervisor CCC build gates).@ckb-ccc/*catalog versions for the migrated protocol packages.