UDT part for @ckb-ccc/rgbpp #338
Conversation
|
✅ Deploy Preview for appccc ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for apiccc ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for liveccc ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Summary of ChangesHello @fghdotio, 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 significantly expands the project's capabilities by introducing a dedicated package for the RGB++ protocol. The changes enable the integration of Bitcoin-based User Defined Tokens (UDTs) with the CKB blockchain, providing necessary script definitions, utility functions for Bitcoin interactions, and a client for managing UDT issuance. This lays the groundwork for advanced cross-chain asset management between Bitcoin and CKB. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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
|
✅ Deploy Preview for docsccc ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![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 new package @ckb-ccc/rgbpp with UDT (User Defined Token) functionality for RGB++. It adds new script definitions for mainnet and testnet, and a lot of new logic for handling RGB++ UDTs, including building CKB transactions, calculating commitments, and interacting with Bitcoin transactions. The code is well-structured, but I've found a few issues, including a critical bug in transaction construction, potential runtime errors from unsafe array access, and some project configuration problems like duplicate dependencies and risky ESLint rules. I've provided suggestions to fix these issues.
packages/rgbpp/package.json
Outdated
| "@ckb-lumos/base": "^0.22.2", | ||
| "@ckb-lumos/codec": "^0.22.2", |
There was a problem hiding this comment.
There are duplicate @ckb-lumos/base and @ckb-lumos/codec packages defined in devDependencies and dependencies. The versions are also different (^0.22.2 vs ^0.23.0). This can lead to version conflicts and unpredictable behavior. Since they are required at runtime, they should only be in dependencies. Please remove them from devDependencies.
packages/rgbpp/src/udt/index.ts
Outdated
| type: ccc.Script.from({ | ||
| codeHash: params.udtScriptInfo.codeHash, | ||
| hashType: params.udtScriptInfo.hashType, | ||
| args: params.rgbppLiveCells[0].cellOutput.lock.hash(), // unique ID of udt token |
There was a problem hiding this comment.
There is a bug here. params.rgbppLiveCells is used to get the first cell, but this array might contain duplicates and have a different order than rgpbbLiveCells, which is the deduplicated and correctly ordered list of cells used for creating transaction inputs. You should use rgpbbLiveCells[0] to ensure you're using a cell that is actually part of the transaction inputs and to make the UDT's unique ID deterministic.
| args: params.rgbppLiveCells[0].cellOutput.lock.hash(), // unique ID of udt token | |
| args: rgpbbLiveCells[0].cellOutput.lock.hash(), // unique ID of udt token |
| }, | ||
| ], | ||
| "@typescript-eslint/unbound-method": ["error", { ignoreStatic: true }], | ||
| "@typescript-eslint/no-unsafe-member-access": "off", |
There was a problem hiding this comment.
Disabling @typescript-eslint/no-unsafe-member-access is highly discouraged as it can hide potential runtime errors by allowing member access on any types without checks. This undermines the benefits of TypeScript's type safety. It's better to handle any types explicitly (e.g., with type guards or assertions) or, if necessary, disable the rule on a case-by-case basis with an inline comment explaining why it's safe. Consider re-enabling this rule or setting it to "warn" to avoid introducing bugs.
| "@typescript-eslint/no-unsafe-member-access": "off", | |
| "@typescript-eslint/no-unsafe-member-access": "warn", |
| /** | ||
| * @see https://prettier.io/docs/configuration | ||
| * @type {import("prettier").Config} | ||
| */ | ||
| const config = { | ||
| singleQuote: false, | ||
| trailingComma: "all", | ||
| plugins: [require.resolve("prettier-plugin-organize-imports")], | ||
| }; | ||
|
|
||
| module.exports = config; |
There was a problem hiding this comment.
This Prettier configuration file (prettier.config.cjs) seems redundant because a .prettierrc file with similar settings already exists in the same directory. Having multiple configuration files for the same tool can be confusing and lead to inconsistencies. It's recommended to consolidate into a single configuration file. Please consider removing this file and keeping only .prettierrc.
packages/rgbpp/src/udt/index.ts
Outdated
| } | ||
|
|
||
| if (rgbppCells.length !== 0) { | ||
| console.log("Using existing RGB++ cell"); |
There was a problem hiding this comment.
Using console.log for logging (here and on lines 132 and 144) in a library package is generally not recommended. It can pollute the console of the consuming application and isn't configurable (e.g., to control log levels or redirect output). Consider using a dedicated, configurable logging library (like debug or pino) or passing in an optional logger instance to the client.
| tx.addCellDeps( | ||
| params.udtScriptInfo.cellDeps[0].cellDep, | ||
| uniqueTypeInfo.cellDeps[0].cellDep, | ||
| ); |
There was a problem hiding this comment.
Accessing cellDeps[0] directly without checking if the cellDeps array is empty can lead to a runtime error if a script happens to have no cell dependencies defined. It's safer to add a check to ensure the dependencies exist before accessing them.
if (!params.udtScriptInfo.cellDeps[0] || !uniqueTypeInfo.cellDeps[0]) {
throw new Error("Missing cell dependencies for UDT or UniqueType script");
}
tx.addCellDeps(
params.udtScriptInfo.cellDeps[0].cellDep,
uniqueTypeInfo.cellDeps[0].cellDep,
);… add cellDeps array null checks
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new package @ckb-ccc/rgbpp to handle RGB++ related logic, including UDT issuance. It adds new script definitions for mainnet and testnet, and a comprehensive set of types, utilities, and a client for RGB++. The overall structure is well-designed and the implementation covers many aspects of the RGB++ protocol. I've found a few issues, mainly in utility functions, related to incorrect length checks and handling of hex strings, which could lead to bugs. I've also pointed out a magic number that should be replaced by a constant. The suggested changes will improve the correctness and maintainability of the new package.
| export function getTxIdFromScriptArgs(args: ccc.Hex): string { | ||
| if (args.length < 32 * 2) { | ||
| throw new Error("Lock args length is invalid"); | ||
| } | ||
|
|
||
| return trimHexPrefix( | ||
| reverseHexByteOrder(args.substring(args.length - 32 * 2) as ccc.Hex), | ||
| ); | ||
| } |
There was a problem hiding this comment.
This function has two issues:
- The length check
args.length < 32 * 2is incorrect becauseargsis accc.Hexstring which includes the0xprefix. The check should be on the length of the hex characters, so it should beargs.length < 32 * 2 + 2. - The implementation is inconsistent with
updateScriptArgsWithTxIdwhich operates on a trimmed hex string. It's better to be consistent.
Here is a suggested fix that addresses both points.
export function getTxIdFromScriptArgs(args: ccc.Hex): string {
const trimmed = trimHexPrefix(args);
if (trimmed.length < 32 * 2) {
throw new Error("Lock args length is invalid");
}
return trimHexPrefix(
reverseHexByteOrder(
prependHexPrefix(trimmed.substring(trimmed.length - 32 * 2)),
),
);
}| export function getTxIndexFromScriptArgs(args: ccc.Hex): number { | ||
| if (args.length < 32 * 2) { | ||
| throw new Error("Lock args length is invalid"); | ||
| } | ||
|
|
||
| return parseInt( | ||
| reverseHexByteOrder(trimHexPrefix(args.substring(0, 8)) as ccc.Hex), | ||
| 16, | ||
| ); | ||
| } |
There was a problem hiding this comment.
This function has two issues, likely from copy-pasting getTxIdFromScriptArgs:
- The length check
args.length < 32 * 2is incorrect. It should check for the presence of a 4-byte index, not a 32-byte txId. The check should betrimHexPrefix(args).length < 4 * 2. args.substring(0, 8)is incorrect becauseargsincludes the0xprefix. This will slice part of the prefix and part of the hex. It should operate on the trimmed hex string.
Here is a suggested fix that addresses both points.
export function getTxIndexFromScriptArgs(args: ccc.Hex): number {
const trimmed = trimHexPrefix(args);
if (trimmed.length < 4 * 2) {
throw new Error("Lock args length is invalid");
}
return parseInt(
reverseHexByteOrder(prependHexPrefix(trimmed.substring(0, 8))),
16,
);
}| throw new Error("Unsupported output"); | ||
| } | ||
|
|
||
| const minUtxoSatoshi = 546; |
There was a problem hiding this comment.
The value 546 for minUtxoSatoshi is a magic number. To improve readability and maintainability, please import and use the DEFAULT_DUST_LIMIT constant, which is already defined in ../configs/scripts/index.js.
You'll need to add this import at the top of the file:
import { DEFAULT_DUST_LIMIT } from "../configs/scripts/index.js";| const minUtxoSatoshi = 546; | |
| const minUtxoSatoshi = DEFAULT_DUST_LIMIT; |
| if (argsLength < (32 + 2) * 2) { | ||
| throw new Error("Lock args length is invalid"); | ||
| } |
There was a problem hiding this comment.
The length check argsLength < (32 + 2) * 2 is likely incorrect. The value (32 + 2) * 2 evaluates to 68. A 32-byte hex string is 64 characters long. The purpose of this check is probably to ensure the args string is long enough to contain a 32-byte txId. A more accurate check would be argsLength < 32 * 2.
| if (argsLength < (32 + 2) * 2) { | |
| throw new Error("Lock args length is invalid"); | |
| } | |
| if (argsLength < 32 * 2) { | |
| throw new Error("Lock args length is invalid"); | |
| } |
1 participant
No description provided.