Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request undertakes a significant refactoring of the User Defined Token (UDT) handling within the iCKB project. The primary goal is to transition from a custom, homegrown UDT infrastructure to leveraging the robust and standardized Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 is a significant and well-executed refactoring that replaces the homegrown UDT infrastructure with CCC's udt.Udt base class. This aligns iCKB with the upstream CCC token abstraction, reducing maintenance and enabling built-in balance completion. The changes are consistent across all packages, from deleting old utilities to updating the SDK to construct the new IckbUdt class. I've provided suggestions to improve readability and reduce code duplication, along with a note on a documentation inconsistency that should be updated to reflect the actual implementation, in line with repository guidelines.
| const ickbUdt = new IckbUdt( | ||
| // xUDT code cell OutPoint: use known constants for mainnet/testnet, | ||
| // fall back to first cellDep's outPoint for devnet. | ||
| network | ||
| ? (network === "mainnet" ? MAINNET_XUDT_CODE : TESTNET_XUDT_CODE) | ||
| : d.udt.cellDeps[0]?.outPoint ?? ((): never => { throw new Error("devnet config missing xUDT code cell outPoint in udt.cellDeps"); })(), | ||
| IckbUdt.typeScriptFrom( | ||
| ccc.Script.from(d.udt.script), | ||
| ccc.Script.from(d.logic.script), | ||
| ), | ||
| // iCKB Logic code cell OutPoint: same pattern as above. | ||
| network | ||
| ? (network === "mainnet" ? MAINNET_LOGIC_CODE : TESTNET_LOGIC_CODE) | ||
| : d.logic.cellDeps[0]?.outPoint ?? ((): never => { throw new Error("devnet config missing Logic code cell outPoint in logic.cellDeps"); })(), | ||
| d.logic.script, | ||
| d.logic.cellDeps, | ||
| dao, | ||
| ickbUdt, | ||
| ); |
There was a problem hiding this comment.
While this pattern of using a nullish coalescing operator with an immediately-invoked function expression that throws an error is functionally correct, it can be a bit dense and hard to read for some developers. Consider refactoring this to a more traditional if check for clarity. This would make the intent of throwing an error on missing configuration more explicit and easier to understand at a glance.
const xudtCode = network
? network === "mainnet"
? MAINNET_XUDT_CODE
: TESTNET_XUDT_CODE
: d.udt.cellDeps[0]?.outPoint;
if (!xudtCode) {
throw new Error(
'devnet config missing xUDT code cell outPoint in udt.cellDeps',
);
}
const logicCode = network
? network === "mainnet"
? MAINNET_LOGIC_CODE
: TESTNET_LOGIC_CODE
: d.logic.cellDeps[0]?.outPoint;
if (!logicCode) {
throw new Error(
'devnet config missing Logic code cell outPoint in logic.cellDeps',
);
}
const ickbUdt = new IckbUdt(
xudtCode,
IckbUdt.typeScriptFrom(
ccc.Script.from(d.udt.script),
ccc.Script.from(d.logic.script),
),
logicCode,
d.logic.script,
dao,
);There was a problem hiding this comment.
@gemini-code-assist Already refactored — the latest push uses separate xudtCode/logicCode variables with explicit if (!...) checks, exactly as suggested.
There was a problem hiding this comment.
The refactoring looks good. It significantly improves readability and maintainability by making the error-handling logic explicit. This aligns well with best practices for clear and robust code.
Add phase 04 (CCC UDT investigation) and phase 05 (iCKB core UDT refactor) planning documents including research, context, plans, execution summaries, and verification reports. Update PROJECT.md, REQUIREMENTS.md, ROADMAP.md, and STATE.md to reflect the new milestone scope and phase progression.
Add @ckb-ccc/udt as a catalog dependency and wire it into @ickb/core. This provides the udt.Udt base class that IckbUdt will extend.
Rewrite IckbUdtManager as IckbUdt, extending the CCC udt.Udt base class instead of the homegrown UdtManager. The new class: - Accepts code OutPoints (xUDT, Logic) instead of pre-built CellDep arrays - Overrides infoFrom() to value iCKB's three cell representations (xUDT cells, receipt cells, deposit cells) - Overrides addCellDeps() to add individual code deps - Exposes typeScriptFrom() static for computing the UDT type script Also widen DaoManager.isDeposit() to accept CellAny (needed by IckbUdt.infoFrom which works with CellAny, not just Cell).
- Remove udtHandler parameter from LogicManager and OwnedOwnerManager constructors; callers now manage UDT cellDeps via IckbUdt.addCellDeps() - Replace UdtHandler with plain udtScript in OrderManager - Wire getConfig() to construct IckbUdt with code OutPoints, computing the UDT type script dynamically via IckbUdt.typeScriptFrom() - Delete UDT infrastructure from @ickb/utils (UdtHandler, UdtManager, UdtCell, ErrorTransactionInsufficientCoin) — all superseded by CCC
phroi
force-pushed
the
refactor/adopt-ccc-udt
branch
from
7b3b168 to
0ee8e2a
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This is a major and well-executed refactoring that replaces the homegrown UDT infrastructure with CCC's udt.Udt base class. The changes are comprehensive, touching multiple packages and aligning iCKB with the upstream CCC token abstraction. The code is cleaner and more maintainable. I've found one opportunity for a performance improvement in the infoFrom method by parallelizing network requests.
|
LGTM Phroi %219 |
1 participant
Why
Replace the homegrown UDT infrastructure (
UdtHandler,UdtManager) with CCC'sudt.Udtbase class. This aligns iCKB with the upstream CCC token abstraction, reducing maintenance surface and enabling CCC's built-in UDT balance completion.Changes
@ickb/core: RewriteIckbUdtManagerasIckbUdtextendingudt.Udt, accepting code OutPoints instead of CellDep arrays. OverrideinfoFrom()to value xUDT, receipt, and deposit cells. RemoveudtHandlerfromLogicManagerandOwnedOwnerManager.@ickb/dao: WidenDaoManager.isDeposit()to acceptCellAny.@ickb/order: ReplaceUdtHandlerwith plainudtScriptinOrderManager.@ickb/utils: DeleteUdtHandler,UdtManager,UdtCell,ErrorTransactionInsufficientCoin.@ickb/sdk: WiregetConfig()to constructIckbUdtwith code OutPoints, computing the UDT type script dynamically.