Skip to content

Conversation

@ashuralyk
Copy link
Contributor

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.

@changeset-bot
Copy link

changeset-bot bot commented Jul 3, 2025

🦋 Changeset detected

Latest 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

@netlify
Copy link

netlify bot commented Jul 3, 2025

Deploy Preview for liveccc ready!

Name Link
🔨 Latest commit 3ef9c60
🔍 Latest deploy log https://app.netlify.com/projects/liveccc/deploys/68b96e8d1c9ec80008953eca
😎 Deploy Preview https://deploy-preview-235--liveccc.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 9 (no change from production)
Accessibility: 88 (no change from production)
Best Practices: 92 (🔴 down 8 from production)
SEO: 100 (no change from production)
PWA: -
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify project configuration.

@netlify
Copy link

netlify bot commented Jul 3, 2025

Deploy Preview for apiccc ready!

Name Link
🔨 Latest commit 3ef9c60
🔍 Latest deploy log https://app.netlify.com/projects/apiccc/deploys/68b96e8d84708a0007474520
😎 Deploy Preview https://deploy-preview-235--apiccc.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 90 (🟢 up 18 from production)
Accessibility: 90 (no change from production)
Best Practices: 92 (no change from production)
SEO: 82 (no change from production)
PWA: -
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify project configuration.

@netlify
Copy link

netlify bot commented Jul 3, 2025

Deploy Preview for appccc ready!

Name Link
🔨 Latest commit 3ef9c60
🔍 Latest deploy log https://app.netlify.com/projects/appccc/deploys/68b96e8dc8872e000821371f
😎 Deploy Preview https://deploy-preview-235--appccc.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 51 (🔴 down 32 from production)
Accessibility: 88 (no change from production)
Best Practices: 92 (🔴 down 8 from production)
SEO: 100 (no change from production)
PWA: -
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify project configuration.

@netlify
Copy link

netlify bot commented Jul 3, 2025

Deploy Preview for docsccc ready!

Name Link
🔨 Latest commit 3ef9c60
🔍 Latest deploy log https://app.netlify.com/projects/docsccc/deploys/68b96e8d5fba600008ae58ec
😎 Deploy Preview https://deploy-preview-235--docsccc.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 69 (🟢 up 8 from production)
Accessibility: 88 (no change from production)
Best Practices: 92 (no change from production)
SEO: 92 (no change from production)
PWA: -
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify project configuration.

@ashuralyk ashuralyk marked this pull request as ready for review July 4, 2025 04:27
@Hanssen0
Copy link
Member

Hanssen0 commented Jul 4, 2025

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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.

Comment on lines 164 to 63
if (signatures.length < this.multisig.threshold) {
throw new Error("Not enough signature slots to threshold");
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.

Suggested change
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");
}

Copy link
Member

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?

Comment on lines 143 to 149
return hexConcat(
"0x00",
hexFrom(mustMatch.toString(16)),
hexFrom(threshold.toString(16)),
hexFrom(pubkeyBlake160Hashes.length.toString(16)),
...pubkeyBlake160Hashes,
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
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,
);

Copy link
Member

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.

Comment on lines 227 to 231
if (signatures.length !== this.multisig.threshold) {
if (restrict) {
throw new Error(
"Bad multisig witness: not enough signatures to threshold",
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The error message "Bad multisig witness: not enough signatures to threshold" is not informative enough. Consider including the actual number of signatures found and the expected threshold to aid debugging.

Copy link
Member

@Hanssen0 Hanssen0 left a 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);
Copy link
Member

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?

Copy link
Contributor Author

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(
Copy link
Member

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.

* await tx.prepareMultisigWitness(script, multisigMetadata, threshold, client);
* ```
*/
async prepareMultisigWitness(
Copy link
Member

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

* ```
*/

export function multisigMetadataFromPubkeys(
Copy link
Member

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.

* ```
*/

static async fromKnownMultisigScript(
Copy link
Member

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?:
Copy link
Member

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[];
Copy link
Member

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.

@ashuralyk
Copy link
Contributor Author

/gemini review

@ashuralyk ashuralyk requested a review from Hanssen0 July 5, 2025 06:54
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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.

Comment on lines 180 to 244
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;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.

Comment on lines 58 to 67
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;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This findKnownScript implementation is identical to the one in clientPublicTestnet.ts. To adhere to the DRY (Don't Repeat Yourself) principle, consider abstracting this logic into a common base class.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same opinion

Comment on lines 57 to 68
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;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This findKnownScript implementation is identical to the one in clientPublicMainnet.ts. To adhere to the DRY (Don't Repeat Yourself) principle, consider abstracting this logic into a common base class.

}

// Prepare signature placeholder
const emptySignature = hexFrom(Array.from(new Array(65), () => 0));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Consider defining emptySignature as a constant at the module level to avoid duplication with line 281.

Suggested change
const emptySignature = hexFrom(Array.from(new Array(65), () => 0));
const emptySignature = hexFrom(new Uint8Array(65).fill(0));

Copy link
Member

@Hanssen0 Hanssen0 left a 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?:
Copy link
Member

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) {
Copy link
Member

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 {
Copy link
Member

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;
Copy link
Member

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);
Copy link
Member

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.

@Hanssen0
Copy link
Member

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.

Suggested change
.match(/.{1,130}/g)
.match(/.{130}/g)

const signatures =
witness.lock
.slice(this.multisigInfo.metadata.length)
.match(/.{1,130}/g)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.

Suggested change
.match(/.{1,130}/g)
.match(/.{130}/g)

Comment on lines 61 to 68
const scriptInfo = Object.values(this.scripts).find((scriptInfo) => {
if (
scriptInfo?.codeHash === script.codeHash &&
scriptInfo.hashType === script.hashType
) {
return scriptInfo;
}
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The callback function for find can be simplified to a single boolean expression for better readability and conciseness.

    const scriptInfo = Object.values(this.scripts).find(
      (scriptInfo) =>
        scriptInfo?.codeHash === script.codeHash &&
        scriptInfo.hashType === script.hashType,
    );

Comment on lines 61 to 68
const scriptInfo = Object.values(this.scripts).find((scriptInfo) => {
if (
scriptInfo?.codeHash === script.codeHash &&
scriptInfo.hashType === script.hashType
) {
return scriptInfo;
}
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The callback function for find can be simplified to a single boolean expression for better readability and conciseness.

    const scriptInfo = Object.values(this.scripts).find(
      (scriptInfo) =>
        scriptInfo?.codeHash === script.codeHash &&
        scriptInfo.hashType === script.hashType,
    );

@ashuralyk
Copy link
Contributor Author

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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.

Comment on lines 205 to 217
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)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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))) {

@ashuralyk ashuralyk requested a review from Hanssen0 September 4, 2025 04:30
@ashuralyk ashuralyk force-pushed the feat/multisig-singer branch from 9cbb141 to 7ab86a8 Compare September 4, 2025 06:52
@ashuralyk
Copy link
Contributor Author

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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.

Comment on lines +26 to +91
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;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

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;
  }

Comment on lines 197 to 240
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;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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;
  }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants