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.
packages/rgbpp/src/udt/index.ts
Outdated
| 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.
packages/rgbpp/src/utils/script.ts
Outdated
| 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)),
),
);
}
packages/rgbpp/src/utils/script.ts
Outdated
| 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; |
packages/rgbpp/src/utils/script.ts
Outdated
| 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"); | |
| } |
| "license": "ISC", | ||
| "devDependencies": { | ||
| "@ckb-ccc/spore": "workspace:*", | ||
| "@ckb-ccc/udt": "workspace:*", |
There was a problem hiding this comment.
UDT imported, reasonable, where is it used?
There was a problem hiding this comment.
The udt part was extracted from a fully implemented rgbpp package. This package.json is complete, but the code referencing udt is not included in this PR.
|
Nice PR @fghdotio, great work there!! 💪 I was looking to see how it uses the current CCC UDT implementation, but it does not use it, it just import it Not depending on the current UDT implementation may be a good thing cause we are trying to improve it. For example see #261 / #333 and more to come to allow tokens like iCKB to be handled correctly. That said, it currently has quite a bit of duplication with the current CCC UDT, so it could make sense to:
Keep up the great work, Phroi |
Thanks for the kind words and the feedback @phroi! 🙌 |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new package, @ckb-ccc/rgbpp, to support RGB++ protocol interactions on CKB. It includes definitions for new scripts (RgbppLock, BtcTimeLock), a ScriptManager for handling them, Molecule-based type definitions for protocol structures, and various utility functions for building and parsing transactions and scripts related to RGB++. The changes also include a RgbppUdtClient for managing UDT (User Defined Token) operations like issuance.
My review focuses on the correctness and maintainability of the new package. I've identified a few issues, including potential bugs in transaction building logic, type inconsistencies, and use of magic numbers. I've provided suggestions to address these points to improve the robustness of the new package.
| const minUtxoSatoshi = 546; | ||
| const isOpReturnOutput = | ||
| "script" in result && isOpReturnScriptPubkey(result.script); | ||
| if (!isOpReturnOutput && result.value < minUtxoSatoshi) { | ||
| throw new Error("value is less than minUtxoSatoshi"); | ||
| } |
There was a problem hiding this comment.
This check for the dust limit has two issues:
- It uses a hardcoded value
546. A constant should be used for better maintainability. Consider movingDEFAULT_DUST_LIMITfromconfigs/scripts/index.tstoconstants/index.tsand using it here. - It ignores the
minUtxoSatoshiproperty that might be present on theoutputobject. The check should respect this property if it's provided.
| const minUtxoSatoshi = 546; | |
| const isOpReturnOutput = | |
| "script" in result && isOpReturnScriptPubkey(result.script); | |
| if (!isOpReturnOutput && result.value < minUtxoSatoshi) { | |
| throw new Error("value is less than minUtxoSatoshi"); | |
| } | |
| const minUtxoSatoshi = 546; // TODO: use DEFAULT_DUST_LIMIT constant | |
| const isOpReturnOutput = | |
| "script" in result && isOpReturnScriptPubkey(result.script); | |
| if (!isOpReturnOutput && result.value < (result.minUtxoSatoshi ?? minUtxoSatoshi)) { | |
| throw new Error("value is less than minUtxoSatoshi"); | |
| } |
| tx.addCellDeps( | ||
| params.udtScriptInfo.cellDeps[0].cellDep, | ||
| uniqueTypeInfo.cellDeps[0].cellDep, | ||
| ); |
There was a problem hiding this comment.
This code only adds the first cell dependency from udtScriptInfo and uniqueTypeInfo. However, a script can have multiple cell dependencies (as seen with RgbppLock or JoyId scripts). All required cell dependencies should be added to the transaction to prevent creating invalid transactions.
| tx.addCellDeps( | |
| params.udtScriptInfo.cellDeps[0].cellDep, | |
| uniqueTypeInfo.cellDeps[0].cellDep, | |
| ); | |
| tx.addCellDeps( | |
| ...params.udtScriptInfo.cellDeps.map((dep) => dep.cellDep), | |
| ...uniqueTypeInfo.cellDeps.map((dep) => dep.cellDep), | |
| ); |
packages/rgbpp/src/utils/script.ts
Outdated
| export function parseUtxoSealFromScriptArgs(args: ccc.Hex): UtxoSeal { | ||
| return { | ||
| txId: getTxIdFromScriptArgs(args), | ||
| index: getTxIndexFromScriptArgs(args), | ||
| }; | ||
| } |
There was a problem hiding this comment.
The functions parseUtxoSealFromScriptArgs, getTxIdFromScriptArgs, and getTxIndexFromScriptArgs assume the argument structure of an RgbppLock script. They will produce incorrect results if used with BtcTimeLock script arguments. To prevent misuse, it would be better to rename them to be more specific, for example: parseUtxoSealFromRgbppLockArgs.
| constructor( | ||
| public lockScript: ccc.Script, | ||
| public after: ccc.Num, | ||
| public btcTxid: string, |
There was a problem hiding this comment.
For consistency with other fields in this file (e.g., btcTx in RgbppUnlock) and to better reflect that this property holds a hex string, the type of btcTxid should be ccc.Hex instead of string.
| public btcTxid: string, | |
| public btcTxid: ccc.Hex, |
packages/rgbpp/src/utils/script.ts
Outdated
| if (argsBytes.length < 32 + 2) { | ||
| throw new Error("Lock args length is invalid"); | ||
| } |
There was a problem hiding this comment.
The length check argsBytes.length < 32 + 2 seems incorrect. ccc.bytesFrom returns a Uint8Array, and its length is the number of bytes. The + 2 is likely a mistake from considering the "0x" prefix of a hex string, which is not part of the byte array.
Furthermore, getTxIdFromScriptArgs in the same file uses argsBytes.length < 32. These checks should be consistent.
| if (argsBytes.length < 32 + 2) { | |
| throw new Error("Lock args length is invalid"); | |
| } | |
| if (argsBytes.length < 32) { | |
| throw new Error("Lock args length is invalid"); | |
| } |
…lock args parsing, and update `btcTxid` type.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new package, @ckb-ccc/rgbpp, to support RGB++ UDTs on CKB. The changes are extensive, adding new script definitions for mainnet and testnet, a comprehensive set of types, utility functions, and a client for UDT operations like issuance. The overall architecture is well-designed, with good separation of concerns and use of dependency injection. However, I've identified a few issues related to code correctness and maintainability that should be addressed. These include potentially non-deterministic behavior in UDT creation, some code paths that are commented as non-functional, and minor improvements to documentation and configuration.
| try { | ||
| decodeBase58 = bitcoin.address.fromBase58Check(address); | ||
| if (decodeBase58.version === mainnet.pubKeyHash) { | ||
| networkType = NetworkType.MAINNET; | ||
| addressType = AddressType.P2PKH; | ||
| } else if (decodeBase58.version === testnet.pubKeyHash) { | ||
| networkType = NetworkType.TESTNET; | ||
| addressType = AddressType.P2PKH; | ||
| } else if (decodeBase58.version === regtest.pubKeyHash) { | ||
| // do not work | ||
| networkType = NetworkType.REGTEST; | ||
| addressType = AddressType.P2PKH; | ||
| } else if (decodeBase58.version === mainnet.scriptHash) { | ||
| networkType = NetworkType.MAINNET; | ||
| addressType = AddressType.P2SH_P2WPKH; | ||
| } else if (decodeBase58.version === testnet.scriptHash) { | ||
| networkType = NetworkType.TESTNET; | ||
| addressType = AddressType.P2SH_P2WPKH; | ||
| } else if (decodeBase58.version === regtest.scriptHash) { | ||
| // do not work | ||
| networkType = NetworkType.REGTEST; | ||
| addressType = AddressType.P2SH_P2WPKH; | ||
| } | ||
|
|
||
| if (networkType !== undefined && addressType !== undefined) { | ||
| return { | ||
| networkType, | ||
| addressType, | ||
| }; | ||
| } | ||
| } catch (_e) { | ||
| // Do nothing (no need to throw here) | ||
| } | ||
| } |
There was a problem hiding this comment.
The decodeAddress function includes comments // do not work for regtest address decoding (lines 66 and 76). This indicates that these code paths are either broken or not implemented. Leaving non-functional code can lead to bugs and confusion. Please either implement the logic for regtest addresses or remove these unsupported code paths.
packages/rgbpp/src/udt/issuance.ts
Outdated
| throw new RgbppValidationError("rgbppLiveCells is empty"); | ||
| } | ||
|
|
||
| const rgbppLiveCells = deduplicateByOutPoint(params.rgbppLiveCells); |
There was a problem hiding this comment.
The unique ID for the UDT is derived from the first cell in rgbppLiveCells (rgbppLiveCells[0]). However, the order of params.rgbppLiveCells is not guaranteed, which could lead to non-deterministic UDT IDs if the same set of cells is provided in a different order. To ensure the UDT ID is always the same for the same set of input cells, you should sort the cells by their outpoint before using them. This will guarantee deterministic behavior.
const rgbppLiveCells = deduplicateByOutPoint(
[...params.rgbppLiveCells].sort((a, b) => {
const txHashCmp = a.outPoint.txHash.localeCompare(b.outPoint.txHash);
if (txHashCmp !== 0) return txHashCmp;
return a.outPoint.index.cmp(b.outPoint.index);
}),
);| /** | ||
| * @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.
There seems to be a redundant Prettier configuration. This file (prettier.config.cjs) and .prettierrc in the same directory define the same settings. Having multiple configuration files can lead to confusion and maintenance overhead. It's recommended to consolidate them into a single file to ensure consistency.
| /* | ||
| https://github.com/nervosnetwork/rfcs/blob/master/rfcs/0022-transaction-structure/0022-transaction-structure.md#type-id | ||
|
|
||
| There are two ways to create a new cell with a specific type id. | ||
|
|
||
| 1. Create a transaction which uses any out point as tx.inputs[0] and has a output cell whose type script is Type ID. The output cell's type script args is the hash of tx.inputs[0] and its output index. Because any out point can only be used once as an input, tx.inputs[0] and thus the new type id must be different in each creation transaction. | ||
| 2. Destroy an old cell with a specific type id and create a new cell with the same type id in the same transaction. | ||
| */ |
There was a problem hiding this comment.
This comment block explaining the logic for buildUniqueTypeScript is not in JSDoc format, making it less readable and inconsistent with the documentation style of other methods in this class. For better maintainability and to allow documentation generation tools to parse it correctly, please convert this into a standard JSDoc comment.
…gtest` address type detection, update JSDoc comments
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new package @ckb-ccc/rgbpp for handling RGB++ UDTs, which is a significant and well-structured addition. The changes include new configurations for mainnet and testnet, a comprehensive set of types, utility functions, and a client for interacting with RGB++ assets. While the overall implementation is solid, I've identified a critical bug in the commitment calculation logic and a high-severity issue related to potential precision loss in amount calculations. Addressing these points will be crucial for the correctness and reliability of the new package.
| export const buildBtcRgbppOutputs = async ( | ||
| ckbPartialTx: ccc.Transaction, | ||
| btcChangeAddress: string, | ||
| receiverBtcAddresses: string[], | ||
| btcDustLimit: number, | ||
| rgbppUdtClient: RgbppUdtClient, | ||
| ): Promise<TxOutput[]> => { | ||
| const commitment = calculateCommitment(ckbPartialTx); | ||
|
|
||
| const rgbppLockScriptTemplate = | ||
| await rgbppUdtClient.rgbppLockScriptTemplate(); | ||
| const btcTimeLockScriptTemplate = | ||
| await rgbppUdtClient.btcTimeLockScriptTemplate(); | ||
|
|
||
| const outputs: InitOutput[] = []; | ||
| let lastCkbTypedOutputIndex = -1; | ||
| ckbPartialTx.outputs.forEach((output, index) => { | ||
| // If output.type is not null, then the output.lock must be RGB++ Lock or BTC Time Lock | ||
| if (output.type) { | ||
| if ( | ||
| !isUsingOneOfScripts(output.lock, [ | ||
| rgbppLockScriptTemplate, | ||
| btcTimeLockScriptTemplate, | ||
| ]) | ||
| ) { | ||
| throw new Error("Invalid cell lock"); | ||
| } | ||
| lastCkbTypedOutputIndex = index; | ||
| } | ||
|
|
||
| // If output.lock is RGB++ Lock, generate a corresponding output in outputs | ||
| if (isSameScriptTemplate(output.lock, rgbppLockScriptTemplate)) { | ||
| outputs.push({ | ||
| fixed: true, | ||
| // Out-of-range index indicates this is a RGB++ change output returning to the BTC address | ||
| address: receiverBtcAddresses[index] ?? btcChangeAddress, | ||
| value: btcDustLimit, | ||
| minUtxoSatoshi: btcDustLimit, | ||
| }); | ||
| } | ||
| }); | ||
|
|
||
| if (lastCkbTypedOutputIndex < 0) { | ||
| throw new Error("Invalid outputs"); | ||
| } | ||
|
|
||
| if (!isCommitmentMatched(commitment, ckbPartialTx, lastCkbTypedOutputIndex)) { | ||
| throw new Error("Commitment mismatch"); | ||
| } | ||
|
|
||
| // place the commitment as the first output | ||
| outputs.unshift({ | ||
| data: commitment, | ||
| value: 0, | ||
| fixed: true, | ||
| }); | ||
|
|
||
| return outputs.map((output) => convertToOutput(output)); | ||
| }; |
There was a problem hiding this comment.
The RGB++ commitment is being calculated incorrectly. It is currently computed based on the entire ckbPartialTx, which might include non-RGBPP outputs like change cells. According to the RGB++ protocol, the commitment should only cover the "virtual" part of the transaction, which ends at the lastCkbTypedOutputIndex.
The current logic first calculates a commitment from the full transaction, then attempts to verify it against a commitment from a sliced transaction. This check will incorrectly fail if there are any outputs after the last RGB++-related output. More importantly, the incorrect commitment from the full transaction is then embedded in the Bitcoin transaction's OP_RETURN output.
To fix this, you should first identify the lastCkbTypedOutputIndex, then create a new transaction object sliced to that index, and finally calculate the commitment from this sliced transaction. The redundant isCommitmentMatched check should be removed, as you are constructing the commitment, not verifying it.
export const buildBtcRgbppOutputs = async (
ckbPartialTx: ccc.Transaction,
btcChangeAddress: string,
receiverBtcAddresses: string[],
btcDustLimit: number,
rgbppUdtClient: RgbppUdtClient,
): Promise<TxOutput[]> => {
const rgbppLockScriptTemplate =
await rgbppUdtClient.rgbppLockScriptTemplate();
const btcTimeLockScriptTemplate =
await rgbppUdtClient.btcTimeLockScriptTemplate();
const outputs: InitOutput[] = [];
let lastCkbTypedOutputIndex = -1;
ckbPartialTx.outputs.forEach((output, index) => {
// If output.type is not null, then the output.lock must be RGB++ Lock or BTC Time Lock
if (output.type) {
if (
!isUsingOneOfScripts(output.lock, [
rgbppLockScriptTemplate,
btcTimeLockScriptTemplate,
])
) {
throw new Error("Invalid cell lock");
}
lastCkbTypedOutputIndex = index;
}
// If output.lock is RGB++ Lock, generate a corresponding output in outputs
if (isSameScriptTemplate(output.lock, rgbppLockScriptTemplate)) {
outputs.push({
fixed: true,
// Out-of-range index indicates this is a RGB++ change output returning to the BTC address
address: receiverBtcAddresses[index] ?? btcChangeAddress,
value: btcDustLimit,
minUtxoSatoshi: btcDustLimit,
});
}
});
if (lastCkbTypedOutputIndex < 0) {
throw new Error("Invalid outputs");
}
const rgbppPartialTx = ccc.Transaction.from({
inputs: ckbPartialTx.inputs,
outputs: ckbPartialTx.outputs.slice(0, lastCkbTypedOutputIndex + 1),
outputsData: ckbPartialTx.outputsData.slice(
0,
lastCkbTypedOutputIndex + 1,
),
});
const commitment = calculateCommitment(rgbppPartialTx);
// place the commitment as the first output
outputs.unshift({
data: commitment,
value: 0,
fixed: true,
});
return outputs.map((output) => convertToOutput(output));
};
packages/rgbpp/src/udt/issuance.ts
Outdated
| args: rgbppLiveCells[0].cellOutput.lock.hash(), // unique ID of udt token | ||
| }), | ||
| }, | ||
| ccc.numLeToBytes(params.amount * BigInt(10 ** params.token.decimal), 16), |
There was a problem hiding this comment.
There's a potential for precision loss when calculating the token amount. The expression 10 ** params.token.decimal is evaluated as a number, which can lead to an imprecise floating-point representation if params.token.decimal is large (e.g., greater than 21). When BigInt() is subsequently called on this potentially imprecise number, it can result in an incorrect final amount being encoded.
To ensure precision and correctness, you should perform the exponentiation using BigInt arithmetic from the start.
| ccc.numLeToBytes(params.amount * BigInt(10 ** params.token.decimal), 16), | |
| ccc.numLeToBytes(params.amount * (10n ** BigInt(params.token.decimal)), 16), |
…ctions directly and fix BigInt exponentiation for token decimals.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new package @ckb-ccc/rgbpp to support RGB++ UDT functionalities. This includes adding new script definitions for mainnet and testnet, a comprehensive set of types, utility functions for Bitcoin and CKB interactions, and client classes to orchestrate RGB++ operations like UDT issuance. The overall structure is well-organized with clear separation of concerns. My feedback focuses on improving code consistency and reducing duplication for better maintainability.
| export class ExtraCommitmentDataCCC extends mol.Entity.Base< | ||
| ExtraCommitmentDataLike, | ||
| ExtraCommitmentDataCCC | ||
| >() { |
There was a problem hiding this comment.
The class name ExtraCommitmentDataCCC is inconsistent with other classes in this file like RgbppUnlock and BtcTimeLock, which do not have the CCC suffix. For better consistency and readability, it should be renamed to ExtraCommitmentData. Please also update its usages throughout this file.
export class ExtraCommitmentData extends mol.Entity.Base<
ExtraCommitmentDataLike,
ExtraCommitmentData
> {| constructor( | ||
| _networkConfig: NetworkConfig, | ||
| private ckbClient: ccc.Client, | ||
| scriptProvider: IScriptProvider, | ||
| ) { |
There was a problem hiding this comment.
The constructor parameter _networkConfig is not used within the class. It should be removed to simplify the constructor signature and avoid confusion.
constructor(
private ckbClient: ccc.Client,
scriptProvider: IScriptProvider,
) {| async getRgbppScriptInfos(): Promise< | ||
| Record<RgbppScriptName, ccc.ScriptInfo> | ||
| > { | ||
| const [rgbppLock, btcTimeLock, uniqueType] = await Promise.all([ | ||
| this.scriptManager.getKnownScriptInfo(ccc.KnownScript.RgbppLock), | ||
| this.scriptManager.getKnownScriptInfo(ccc.KnownScript.BtcTimeLock), | ||
| this.scriptManager.getKnownScriptInfo(ccc.KnownScript.UniqueType), | ||
| ]); | ||
|
|
||
| return { | ||
| [ccc.KnownScript.RgbppLock]: rgbppLock, | ||
| [ccc.KnownScript.BtcTimeLock]: btcTimeLock, | ||
| [ccc.KnownScript.UniqueType]: uniqueType, | ||
| } as Record<RgbppScriptName, ccc.ScriptInfo>; | ||
| } |
There was a problem hiding this comment.
This method getRgbppScriptInfos is a duplicate of the method with the same name in ScriptManager. To adhere to the DRY (Don't Repeat Yourself) principle and improve maintainability, this method should delegate the call to this.scriptManager.
async getRgbppScriptInfos(): Promise<
Record<RgbppScriptName, ccc.ScriptInfo>
> {
return this.scriptManager.getRgbppScriptInfos();
}
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new package @ckb-ccc/rgbpp for RGB++ protocol integration, including UDT issuance. The changes are extensive, adding new configurations, types, utilities, and services. The overall structure of the new package is well-designed with a clear separation of concerns. My review has identified a few issues, including a critical race condition in transaction construction, a critical unhandled case that could lead to a runtime error, a high-severity issue with the build script, and a minor issue with a misleading comment. Addressing these points will improve the robustness and correctness of the new package.
| const payment = bitcoin.payments.embed({ data: [data] }); | ||
| return payment.output!; |
There was a problem hiding this comment.
The function dataToOpReturnScriptPubkey uses a non-null assertion ! on payment.output. However, bitcoinjs-lib's payments.embed function will return a payment object with an undefined output if the provided data is empty. If this function is called with an empty string or an empty buffer, it will cause a runtime error. It's important to handle this case by adding a check for empty data to prevent crashes.
const payment = bitcoin.payments.embed({ data: [data] });
if (!payment.output) {
throw new Error("Failed to create OP_RETURN script. Data cannot be empty.");
}
return payment.output;
packages/rgbpp/src/udt/issuance.ts
Outdated
| await Promise.all( | ||
| rgbppLiveCells.map(async (cell) => { | ||
| const cellInput = ccc.CellInput.from({ | ||
| previousOutput: cell.outPoint, | ||
| }); | ||
| await cellInput.completeExtraInfos(ckbClient); | ||
| tx.inputs.push(cellInput); | ||
| }), | ||
| ); |
There was a problem hiding this comment.
Using Promise.all with map to asynchronously process rgbppLiveCells and push them to tx.inputs can result in a non-deterministic order of inputs in the transaction. The order of inputs is critical for both the transaction hash and the generation of the unique type ID. A race condition here could lead to unpredictable behavior and transaction validation failures. To ensure a deterministic order, you should process the cells sequentially using a for...of loop.
for (const cell of rgbppLiveCells) {
const cellInput = ccc.CellInput.from({
previousOutput: cell.outPoint,
});
await cellInput.completeExtraInfos(ckbClient);
tx.inputs.push(cellInput);
}
packages/rgbpp/package.json
Outdated
| }, | ||
| "scripts": { | ||
| "test": "jest", | ||
| "build": "rimraf ./dist && rimraf ./dist.commonjs && tsc && tsc --project tsconfig.commonjs.json && copyfiles -u 2 misc/basedirs/**/* .", |
There was a problem hiding this comment.
The build script includes copyfiles -u 2 misc/basedirs/**/* .. However, the misc directory does not exist in this package. This command will likely fail or do nothing, which suggests a misconfiguration or a copy-paste error from another package. If this copyfiles command is not needed, it should be removed to avoid build failures or unexpected behavior. If it is needed, the missing misc directory and its contents should be added.
| EasyToDiscoverType = "EasyToDiscoverType", | ||
| TimeLock = "TimeLock", | ||
|
|
||
| // RGB++ related scripts (default using Bitcoin Testnet3) |
There was a problem hiding this comment.
The comment // RGB++ related scripts (default using Bitcoin Testnet3) is a bit misleading, as these KnownScript entries are generic and used for both mainnet and testnet configurations. A more general comment would be clearer and avoid potential confusion.
| // RGB++ related scripts (default using Bitcoin Testnet3) | |
| // RGB++ related scripts |
2 participants
No description provided.