-
Notifications
You must be signed in to change notification settings - Fork 31
feat: add SignerCkbMultisig module and dependent features #235
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 3ef9c60 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
✅ Deploy Preview for liveccc 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 appccc ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for docsccc ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces the SignerCkbMultisig module, enhancing multisig transaction capabilities. The changes include new helper functions for multisig metadata and transaction preparation. The review focuses on correctness and security, with suggestions to improve hex string conversion and witness validation.
| if (signatures.length < this.multisig.threshold) { | ||
| throw new Error("Not enough signature slots to threshold"); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The check signatures.length < this.multisig.threshold is insufficient. If the witness is malformed to have more signature slots than the threshold, this check would pass, leading to unexpected behavior. It's safer to check for exact equality (signatures.length !== this.multisig.threshold) to ensure the witness structure is valid.
| if (signatures.length < this.multisig.threshold) { | |
| throw new Error("Not enough signature slots to threshold"); | |
| } | |
| if (signatures.length !== this.multisig.threshold) { | |
| throw new Error("The number of signature slots in witness must match the threshold"); | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if it's necessary. Does the script fail if the signatures exceed the threshold?
packages/core/src/ckb/script.ts
Outdated
| return hexConcat( | ||
| "0x00", | ||
| hexFrom(mustMatch.toString(16)), | ||
| hexFrom(threshold.toString(16)), | ||
| hexFrom(pubkeyBlake160Hashes.length.toString(16)), | ||
| ...pubkeyBlake160Hashes, | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The number.toString(16) conversion might lead to single-character hex strings. To ensure correct formatting, pad the hex string with a leading zero if necessary.
| return hexConcat( | |
| "0x00", | |
| hexFrom(mustMatch.toString(16)), | |
| hexFrom(threshold.toString(16)), | |
| hexFrom(pubkeyBlake160Hashes.length.toString(16)), | |
| ...pubkeyBlake160Hashes, | |
| ); | |
| return hexConcat( | |
| "0x00", | |
| hexFrom(mustMatch.toString(16).padStart(2, '0')), | |
| hexFrom(threshold.toString(16).padStart(2, '0')), | |
| hexFrom(pubkeyBlake160Hashes.length.toString(16).padStart(2, '0')), | |
| ...pubkeyBlake160Hashes, | |
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think using numToBytes will be better here, to improve readability and avoid confusion.
| if (signatures.length !== this.multisig.threshold) { | ||
| if (restrict) { | ||
| throw new Error( | ||
| "Bad multisig witness: not enough signatures to threshold", | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution! This PR implemented the multisig feature correctly and also added some useful utilities. There are still some improvements that can be made to the interface. Please check.
| } | ||
|
|
||
| // Charles sends the transaction | ||
| const txHash = await multisigSigners[2].sendTransaction(tx); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the signatures were not fulfilled, can we add a readable error message to sendTransaction to help locate the problem?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sendTransaction through signerCkbMultisig will automatically check the fulfillment state and throw error if not fit all of requirement threshold.
| threshold, | ||
| firstNthMustMatch, | ||
| ); | ||
| const multisigLockScript = await ccc.Script.fromKnownMultisigScript( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to use SignerCkbMultisig to help construct such an address/script? Maybe we can have a SignerCkbMultisigReadonly and let SignerCkbMultisig extend it.
packages/core/src/ckb/transaction.ts
Outdated
| * await tx.prepareMultisigWitness(script, multisigMetadata, threshold, client); | ||
| * ``` | ||
| */ | ||
| async prepareMultisigWitness( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It appears that this logic is closely coupled to the Multisig Signer. Maybe moving it to the Signer implementation is better than making it in the Transaction
packages/core/src/ckb/script.ts
Outdated
| * ``` | ||
| */ | ||
|
|
||
| export function multisigMetadataFromPubkeys( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest moving to the signerCkbMultisig.ts for better cohesion.
packages/core/src/ckb/script.ts
Outdated
| * ``` | ||
| */ | ||
|
|
||
| static async fromKnownMultisigScript( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest moving to the signerCkbMultisig.ts for better cohesion.
| threshold: number; | ||
| mustMatch: number; | ||
| since?: SinceLike; | ||
| multisigScript?: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to let the Signer support both script versions at the same time? e.g., we can let the Signer return two addresses for getAddresses. Similar to https://github.com/ckb-devrel/ccc/blob/master/packages/core/src/signer/evm/signerEvm.ts
| import { SignerCkbPrivateKey } from "./signerCkbPrivateKey.js"; | ||
|
|
||
| export interface MultisigInfo { | ||
| pubkeys: HexLike[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MultisigInfo was used as Signer.multisig member's type, so I don't think we should use *Like here. Considering adding a MulsigInfoLike type for arg types.
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a SignerCkbMultisig module to simplify creating and signing CKB multisig transactions. The implementation is comprehensive, covering readonly and signing-capable signer classes, along with helper methods on the client and transaction objects. The changes also include new examples demonstrating how to use the new functionality.
My review has identified a high-severity bug in how related scripts are discovered for multisig, which could lead to signing failures for non-default multisig script versions. I've also pointed out several medium-severity issues related to code duplication and opportunities to improve code clarity and efficiency. Addressing these points will further strengthen the robustness and maintainability of this new feature.
| async getRelatedScripts( | ||
| txLike: TransactionLike, | ||
| ): Promise<{ script: Script; cellDeps: CellDepInfo[] }[]> { | ||
| const tx = Transaction.from(txLike); | ||
| const multisig = await this.getAddressObjSecp256k1(); | ||
|
|
||
| const scripts: { script: Script; cellDeps: CellDepInfo[] }[] = []; | ||
| for (const input of tx.inputs) { | ||
| const { | ||
| cellOutput: { lock }, | ||
| } = await input.getCell(this.client); | ||
|
|
||
| if (scripts.some(({ script }) => script.eq(lock))) { | ||
| continue; | ||
| } | ||
|
|
||
| if (lock.eq(multisig.script)) { | ||
| const scriptInfo = await this.client.findKnownScript(lock); | ||
| if (scriptInfo) { | ||
| scripts.push({ | ||
| script: lock, | ||
| cellDeps: scriptInfo.cellDeps, | ||
| }); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return scripts; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The getRelatedScripts function only checks for the default multisig script. If a transaction uses a different (but valid) multisig script, this function will fail to identify it, causing signing and other operations to fail. Fetch all possible multisig scripts using this.multisigInfo.multisigScripts(this.client) and check if the input lock script matches any of them.
| scriptLike: ScriptLike, | ||
| ): Promise<ScriptInfo | undefined> { | ||
| const script = Script.from(scriptLike); | ||
| const scriptInfo = Object.values(this.scripts).find((scriptInfo) => { | ||
| if ( | ||
| scriptInfo?.codeHash === script.codeHash && | ||
| scriptInfo.hashType === script.hashType | ||
| ) { | ||
| return scriptInfo; | ||
| } | ||
| }); | ||
| return scriptInfo ? ScriptInfo.from(scriptInfo) : undefined; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same opinion
| async findKnownScript( | ||
| scriptLike: ScriptLike, | ||
| ): Promise<ScriptInfo | undefined> { | ||
| const script = Script.from(scriptLike); | ||
| const scriptInfo = Object.values(this.scripts).find((scriptInfo) => { | ||
| if ( | ||
| scriptInfo?.codeHash === script.codeHash && | ||
| scriptInfo.hashType === script.hashType | ||
| ) { | ||
| return scriptInfo; | ||
| } | ||
| }); | ||
| return scriptInfo ? ScriptInfo.from(scriptInfo) : undefined; | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| } | ||
|
|
||
| // Prepare signature placeholder | ||
| const emptySignature = hexFrom(Array.from(new Array(65), () => 0)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR provides a functional multisig signer, which is great. However, considering its lack of support for possible devnet/unknown scripts, it's more like a proof of concept than a production-ready tool.
Further design is needed.
| threshold: number; | ||
| mustMatch: number; | ||
| since?: SinceLike; | ||
| multisigScript?: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me, this type constraint seems unfriendly for contract developers. If we want to use an unknown but compatible multisig script, this definition won't allow us to do so.
| public readonly pubkeyBlake160Hashes: Hex[]; | ||
| public readonly metadata: Hex; | ||
|
|
||
| private constructor(multisig: MultisigInfoLike) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The TypeLike to Type conversion should happen in the from method, so that we always have a way to avoid conversion cost.
This is a minor issue.
| if (this.since) { | ||
| const sinceBytes = this.since.toBytes(); | ||
| return hexConcat(metadataBlake160Hash, hexFrom(sinceBytes)); | ||
| } else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest utilizing the early return to make the code more readable by avoiding indents.
| } | ||
|
|
||
| get signType(): SignerSignType { | ||
| return SignerSignType.CkbMultisigSecp256k1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will multisig address have their own signature format? Or is it just the same as an ordinary Secp256k1 signature?
If it's a regular Secp256k1 signature, we shouldn't need a new sign type here.
| txLike: TransactionLike, | ||
| scriptLike: ScriptLike, | ||
| ) { | ||
| const tx = Transaction.from(txLike); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If txLike is not an instance of Transaction, this will cause a new instance to be created, and all modifications here won't be able to affect the original argument.
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a SignerCkbMultisig module to simplify creating and signing CKB multisig transactions. It includes the readonly and signing versions of the multisig signer, along with a MultisigInfo helper class for managing multisig configurations. The changes also extend the Client with a findKnownScript method and add new utility functions like hexConcat. Example usage files for transferring to and from a multisig address are also provided.
My review focuses on improving code correctness and maintainability. I've identified a potential bug in how signatures are parsed from the witness, which could lead to incorrect behavior with malformed data. I've also suggested simplifying some logic for better readability.
| const signatures = | ||
| witness.lock | ||
| .slice(this.multisigInfo.metadata.length) | ||
| .match(/.{1,130}/g) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The regex /.{1,130}/g is too permissive for splitting signatures. It should be /.{130}/g to ensure only full 65-byte (130 hex characters) signatures are matched. Using {1,130} could lead to matching partial data at the end of the string if the witness lock is malformed, causing issues with signature validation.
| .match(/.{1,130}/g) | |
| .match(/.{130}/g) |
| const signatures = | ||
| witness.lock | ||
| .slice(this.multisigInfo.metadata.length) | ||
| .match(/.{1,130}/g) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The regex /.{1,130}/g is too permissive for splitting signatures. It should be /.{130}/g to ensure only full 65-byte (130 hex characters) signatures are matched. Using {1,130} could lead to matching partial data at the end of the string if the witness lock is malformed, causing issues with signature validation.
| .match(/.{1,130}/g) | |
| .match(/.{130}/g) |
| const scriptInfo = Object.values(this.scripts).find((scriptInfo) => { | ||
| if ( | ||
| scriptInfo?.codeHash === script.codeHash && | ||
| scriptInfo.hashType === script.hashType | ||
| ) { | ||
| return scriptInfo; | ||
| } | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| const scriptInfo = Object.values(this.scripts).find((scriptInfo) => { | ||
| if ( | ||
| scriptInfo?.codeHash === script.codeHash && | ||
| scriptInfo.hashType === script.hashType | ||
| ) { | ||
| return scriptInfo; | ||
| } | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a SignerCkbMultisig module to simplify creating and signing multisig transactions on CKB. The changes include the core multisig signer logic, a readonly helper class, updates to the client to find script information, and new examples demonstrating usage. The implementation is comprehensive and well-structured. I've found one potential issue in how related scripts are identified, which could cause problems when dealing with different versions of multisig scripts. My detailed feedback is in the review comment.
| const multisig = await this.getAddressObjSecp256k1(); | ||
|
|
||
| const scripts: { script: Script; cellDeps: CellDepInfo[] }[] = []; | ||
| for (const input of tx.inputs) { | ||
| const { | ||
| cellOutput: { lock }, | ||
| } = await input.getCell(this.client); | ||
|
|
||
| if (scripts.some(({ script }) => script.eq(lock))) { | ||
| continue; | ||
| } | ||
|
|
||
| if (lock.eq(multisig.script)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The getRelatedScripts method currently only identifies inputs that are locked with the default multisig script, because it uses getAddressObjSecp256k1. This could cause the signer to miss inputs that use other valid multisig script versions (e.g., the deprecated Secp256k1Multisig vs. Secp256k1MultisigV2). To ensure all relevant inputs are found, you should use getAddressObjs() to get all possible multisig scripts this signer can handle and check if the input's lock script matches any of them.
const multisigScripts = (await this.getAddressObjs()).map(
(addr) => addr.script,
);
const scripts: { script: Script; cellDeps: CellDepInfo[] }[] = [];
for (const input of tx.inputs) {
const {
cellOutput: { lock },
} = await input.getCell(this.client);
if (scripts.some(({ script }) => script.eq(lock))) {
continue;
}
if (multisigScripts.some((s) => s.eq(lock))) {9cbb141 to
7ab86a8
Compare
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a SignerCkbMultisig module to simplify multisig transaction handling on the CKB network. It includes the core signer logic, a readonly variant, and necessary updates to the client and supporting types. The addition of example files is a great way to demonstrate the new functionality.
My review has identified a critical issue in the signing logic that could result in unsigned inputs when multiple inputs share the same lock script. I've also found an opportunity to improve the performance and clarity of the script-finding logic. I've provided detailed suggestions for both of these points. The rest of the implementation, including the new examples, appears to be well-structured and correct.
| async signOnlyTransaction(txLike: TransactionLike): Promise<Transaction> { | ||
| let lastIndex = -1; | ||
| let tx = Transaction.from(txLike); | ||
| const emptySignature = hexFrom(Array.from(new Array(65), () => 0)); | ||
|
|
||
| for (const { script } of await this.getRelatedScripts(tx)) { | ||
| const index = await tx.findInputIndexByLock(script, this.client); | ||
| if (index === undefined) { | ||
| return tx; | ||
| } | ||
| if (index === lastIndex) { | ||
| continue; | ||
| } else { | ||
| lastIndex = index; | ||
| } | ||
|
|
||
| let witness = tx.getWitnessArgsAt(index); | ||
| if (!witness || !witness.lock?.startsWith(this.multisigInfo.metadata)) { | ||
| tx = await this.prepareTransaction(tx); | ||
| } | ||
|
|
||
| witness = tx.getWitnessArgsAt(index); | ||
| if (!witness || !witness.lock) { | ||
| throw new Error("Multisig witness not prepared"); | ||
| } | ||
|
|
||
| // Signatures array is placed after the multisig metadata, in 65 bytes per signature | ||
| const signatures = | ||
| witness.lock | ||
| .slice(this.multisigInfo.metadata.length) | ||
| .match(/.{1,130}/g) | ||
| ?.map(hexFrom) || []; | ||
| const insertIndex = signatures.findIndex((sig) => sig === emptySignature); | ||
| if (signatures.length !== this.multisigInfo.threshold) { | ||
| throw new Error( | ||
| `Not enough signature slots to threshold (${signatures.length}/${this.multisigInfo.threshold})`, | ||
| ); | ||
| } | ||
| if (insertIndex === -1) { | ||
| // Signatures have been filled | ||
| continue; | ||
| } | ||
|
|
||
| // Empty multisig witness for current signing | ||
| witness.lock = hexConcat( | ||
| this.multisigInfo.metadata, | ||
| ...Array.from( | ||
| new Array(this.multisigInfo.threshold), | ||
| () => emptySignature, | ||
| ), | ||
| ); | ||
| tx.setWitnessArgsAt(index, witness); | ||
| const info = await tx.getSignHashInfo(script, this.client); | ||
| if (!info) { | ||
| continue; | ||
| } | ||
|
|
||
| const signature = await this.signer._signMessage(info.message); | ||
| signatures[insertIndex] = signature; | ||
|
|
||
| witness.lock = hexConcat(this.multisigInfo.metadata, ...signatures); | ||
| tx.setWitnessArgsAt(index, witness); | ||
| } | ||
|
|
||
| return tx; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current implementation of signOnlyTransaction iterates over related scripts and then finds the first input that uses that script. This approach is flawed because if multiple inputs in the transaction use the same multisig lock script, only the first one will ever be found and signed. Subsequent inputs with the same lock will be missed.
The logic should be changed to iterate over all transaction inputs and sign each one that is associated with a related multisig script. This ensures all relevant inputs are correctly signed. The lastIndex check is also redundant because getRelatedScripts already returns unique scripts.
async signOnlyTransaction(txLike: TransactionLike): Promise<Transaction> {
let tx = Transaction.from(txLike);
const emptySignature = hexFrom(new Uint8Array(65));
const relatedScripts = (await this.getRelatedScripts(tx)).map(
({ script }) => script,
);
if (relatedScripts.length === 0) {
return tx;
}
for (let i = 0; i < tx.inputs.length; i++) {
const {
cellOutput: { lock },
} = await tx.inputs[i].getCell(this.client);
if (!relatedScripts.some((script) => script.eq(lock))) {
continue;
}
let witness = tx.getWitnessArgsAt(i);
if (!witness || !witness.lock?.startsWith(this.multisigInfo.metadata)) {
tx = await this.prepareTransaction(tx);
}
witness = tx.getWitnessArgsAt(i);
if (!witness || !witness.lock) {
throw new Error("Multisig witness not prepared");
}
// Signatures array is placed after the multisig metadata, in 65 bytes per signature
const signatures =
witness.lock
.slice(this.multisigInfo.metadata.length)
.match(/.{1,130}/g)
?.map(hexFrom) || [];
const insertIndex = signatures.findIndex((sig) => sig === emptySignature);
if (signatures.length !== this.multisigInfo.threshold) {
throw new Error(
`Not enough signature slots to threshold (${signatures.length}/${this.multisigInfo.threshold})`,
);
}
if (insertIndex === -1) {
// Signatures have been filled
continue;
}
// Empty multisig witness for current signing
witness.lock = hexConcat(
this.multisigInfo.metadata,
...Array(this.multisigInfo.threshold).fill(emptySignature),
);
tx.setWitnessArgsAt(i, witness);
const info = await tx.getSignHashInfo(lock, this.client);
if (!info) {
continue;
}
const signature = await this.signer._signMessage(info.message);
signatures[insertIndex] = signature;
witness.lock = hexConcat(this.multisigInfo.metadata, ...signatures);
tx.setWitnessArgsAt(i, witness);
}
return tx;
}| async getRelatedScripts( | ||
| txLike: TransactionLike, | ||
| ): Promise<{ script: Script; cellDeps: CellDepInfo[] }[]> { | ||
| const tx = Transaction.from(txLike); | ||
| const availableMultisigs = await this.getAddressObjs(); | ||
|
|
||
| const scripts: { script: Script; cellDeps: CellDepInfo[] }[] = []; | ||
| for (const multisig of availableMultisigs) { | ||
| for (const input of tx.inputs) { | ||
| const { | ||
| cellOutput: { lock }, | ||
| } = await input.getCell(this.client); | ||
|
|
||
| if (scripts.some(({ script }) => script.eq(lock))) { | ||
| continue; | ||
| } | ||
|
|
||
| if (lock.eq(multisig.script)) { | ||
| const scriptInfo = await this.client.findKnownScript(lock); | ||
| if (scriptInfo) { | ||
| scripts.push({ | ||
| script: lock, | ||
| cellDeps: scriptInfo.cellDeps, | ||
| }); | ||
| } else { | ||
| if (typeof this.multisigInfo.knownMultisigScript === "string") { | ||
| // Generally, this branch could not be reached | ||
| throw new Error( | ||
| `Unsupported multisig script: ${this.multisigInfo.knownMultisigScript}`, | ||
| ); | ||
| } | ||
| scripts.push({ | ||
| script: lock, | ||
| cellDeps: this.multisigInfo.knownMultisigScript.cellDeps.map( | ||
| (cellDep) => CellDepInfo.from(cellDep), | ||
| ), | ||
| }); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return scripts; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The getRelatedScripts method uses nested loops, iterating over all available multisig addresses for each transaction input. This can be inefficient, especially for transactions with many inputs and for signers with many multisig configurations. The complexity is approximately O(number of multisig scripts * number of inputs).
This can be optimized by first iterating over the inputs to collect all unique lock scripts, and then checking which of those scripts are part of the signer's available multisig scripts. This improves both performance and readability.
async getRelatedScripts(
txLike: TransactionLike,
): Promise<{ script: Script; cellDeps: CellDepInfo[] }[]> {
const tx = Transaction.from(txLike);
const availableMultisigs = await this.getAddressObjs();
if (availableMultisigs.length === 0) {
return [];
}
const scripts: { script: Script; cellDeps: CellDepInfo[] }[] = [];
for (const input of tx.inputs) {
const {
cellOutput: { lock },
} = await input.getCell(this.client);
if (scripts.some(({ script }) => script.eq(lock))) {
continue;
}
if (availableMultisigs.some(({ script }) => script.eq(lock))) {
const scriptInfo = await this.client.findKnownScript(lock);
if (scriptInfo) {
scripts.push({
script: lock,
cellDeps: scriptInfo.cellDeps,
});
} else {
if (typeof this.multisigInfo.knownMultisigScript === "string") {
// Generally, this branch could not be reached
throw new Error(
`Unsupported multisig script: ${this.multisigInfo.knownMultisigScript}`,
);
}
scripts.push({
script: lock,
cellDeps: this.multisigInfo.knownMultisigScript.cellDeps.map(
(cellDep) => CellDepInfo.from(cellDep),
),
});
}
}
}
return scripts;
}
Description
Consider we have multiple versions of multiple signature contract deployed on CKB network, and moreover, construction of multiple signature transaction is relatively complicated, so it's indeed necessary to implement a new signer, which is
SignerCkbMultisig, to provide an easy-to-use approach for multisig.