feat(core): multisig Signers#349
Conversation
🦋 Changeset detectedLatest commit: 7bff329 The changes in this PR will be included in the next version bump. This PR includes changesets to release 20 packages
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 docsccc ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Summary of ChangesHello @Hanssen0, 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 enhances the core library by introducing robust support for CKB multisig transactions. It provides new abstract and concrete signer classes, refactors existing signing mechanisms for better modularity, and improves transaction witness argument handling. The changes aim to make multisig operations more accessible and reliable for developers, complete with illustrative examples. 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
|
![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 multisig signer capabilities, which is a significant feature. The changes include new SignerMultisigCkbPrivateKey and SignerMultisigCkbReadonly classes, along with a new SignerMultisig abstract class. The implementation looks mostly correct, but I've found a few issues:
- A critical bug in
getSignaturesCountthat would cause incorrect signature counting. - A misleading example in
transaction.tsfor a new method. - Some minor issues like a
console.logand an incorrect error message. - I've also pointed out a potential API inconsistency in one of the new example files.
Please address these points to improve the quality and correctness of the code. Overall, good work on adding this complex feature.
✅ Deploy Preview for appccc ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Hanssen0
force-pushed
the
feat/multisig-signer
branch
8 times, most recently
from
2d37e11 to
7e47bbe
Compare
|
Related to #196 @ashuralyk, please review. |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant feature: support for multisig signers in @ckb-ccc/core. It also includes a major dependency upgrade for @noble packages and a migration of the build system to tsdown. The core logic for creating, signing, and aggregating multisig transactions is well-implemented, and the refactoring to accommodate the new dependency versions is clean. My review includes a few suggestions to improve type safety and error message clarity.
Hanssen0
force-pushed
the
feat/multisig-signer
branch
from
7e47bbe to
5cd022e
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces significant new functionality by adding multisig signer support for CKB, a major feature for the core package. It also includes a substantial dependency upgrade for all @noble packages to their latest major versions and a migration of the build system to tsdown. The implementation of the multisig feature is well-structured, separating read-only and private key signing logic, and is accompanied by new tests and examples. The breaking changes from the dependency upgrades seem to be handled correctly throughout the codebase. My review includes a few suggestions to enhance maintainability and correct a minor documentation error.
Hanssen0
force-pushed
the
feat/multisig-signer
branch
from
5cd022e to
858410d
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces significant new functionality for multisig signers in the core package, which is a valuable addition. The implementation is well-structured, with clear abstractions for multisig signers and thorough handling of CKB-specific multisig logic, including witness encoding, signature aggregation, and transaction preparation.
The PR also includes a major version bump for @noble dependencies, and the necessary code adaptations for the new APIs are correctly implemented across the affected modules. The migration of the build process to tsdown is a good modernization step.
I have one suggestion for a minor refactoring to reduce code duplication. Overall, this is a high-quality contribution with good test coverage and examples for the new features.
Hanssen0
force-pushed
the
feat/multisig-signer
branch
3 times, most recently
from
87e10d9 to
a137473
Compare
| * @param txs - The transactions to aggregate. | ||
| * @returns The aggregated transaction. | ||
| */ | ||
| async aggregateTransactions(txs: TransactionLike[]): Promise<Transaction> { |
There was a problem hiding this comment.
What's the scenario for calling transactions aggregation?
There was a problem hiding this comment.
The scenario comes from the current ckb-cli usage. First, one transaction constructor builds the original transaction, then distributes it to others by email/IM. After everybody signs their transaction, respectively, and sends them back to the constructor/aggregator, they merge/finalise the transactions into a final one so that they can send it to the blockchain.
|
I've skimmed all code and tried examples relating to Multisig feature in native running Playground that all of them are passed the trial, and code in this PR conforms all items of merging criteria, so I'll give my approval to it. |
| for (const txLike of txs) { | ||
| const tx = Transaction.from(txLike); | ||
| const multisigWitness = this.decodeWitnessArgsAt(tx, info.position); | ||
|
|
||
| if (!multisigWitness) { | ||
| continue; | ||
| } | ||
|
|
||
| for (const sig of multisigWitness.signatures) { | ||
| try { | ||
| signatures.set(recoverMessageSecp256k1(info.message, sig), sig); | ||
| } catch (_) { | ||
| // Ignore invalid signatures | ||
| } | ||
| if (signatures.size >= this.multisigInfo.threshold) { | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| if (signatures.size >= this.multisigInfo.threshold) { | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
As I understand it, the aggregation logic here doesn't take mustMatch into consideration. The process stops immediately when threshold is met. The must match part might be ignored. Could you please clarify this?
|
Hey @Hanssen0! We're integrating CKB into Rosen Bridge and we are considering Omnilock with multisig auth (flag 0x06) for bridge custody. The witness layout is identical to the standalone multisig script, just wrapped in an OmniLockWitnessLock molecule envelope. Looking at your multisig signers, I noticed the Omnilock variant would duplicate ~46% of SignerMultisigCkbReadonly. The only real differences are:
I put together a refactoring that extracts a shared Branch here: #360 (single commit on top of this PR) What's your take? Phroi |
|
After completing the guard-signing comparison, we are leaning towards ECDSA TSS over Omnilock multisig for the Rosen Bridge CKB integration. Details in the #360 closing comment. Hopefully that multisig signer refactoring in that PR will be still useful for other Omnilock users, |
Hanssen0
force-pushed
the
feat/multisig-signer
branch
3 times, most recently
from
d4a3188 to
4c20867
Compare
|
The |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces CKB multisig signers (SignerMultisigCkbPrivateKey, SignerMultisigCkbReadonly, and MultisigCkbWitness) along with corresponding tests, examples, and documentation. It also upgrades @noble packages to v2 across the workspace, transitions the core package build system to tsdown, and adds Transaction.getWitnessArgsAtUnsafe. The review feedback highlights several critical improvements: resolving a logical bug in multisig transaction signing that can append redundant signatures, adding a length guard check to prevent runtime crashes during witness decoding, utilizing the new hashCkbShort helper to clean up manual slicing, and normalizing recovery bits (v) to ensure compatibility with Ethereum-compatible wallets.
Hanssen0
force-pushed
the
feat/multisig-signer
branch
from
4c20867 to
ef773e2
Compare
|
/gemini review |
Hanssen0
force-pushed
the
feat/multisig-signer
branch
from
ef773e2 to
ba8ad01
Compare
There was a problem hiding this comment.
Code Review
This pull request introduces multisig signers, relaxes @ccc.codec's type restrictions, adds Transaction.getWitnessArgsAtUnsafe, and upgrades @noble dependencies to v2 while migrating the build system to tsdown. The code review highlights critical compatibility issues resulting from the @noble/curves v2 upgrade. Specifically, secp256k1.sign now returns a Signature object rather than a Uint8Array, which will cause runtime errors when calling .slice or accessing array indices in both CKB and Dogecoin signing implementations. Furthermore, secp256k1.verify and secp256k1.recoverPublicKey now require compact 64-byte signatures and recovery bits as separate arguments instead of accepting 65-byte signatures directly.
Hanssen0
force-pushed
the
feat/multisig-signer
branch
from
ba8ad01 to
ad03855
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces CKB multisig signers (SignerMultisigCkbReadonly and SignerMultisigCkbPrivateKey), updates @noble cryptographic dependencies to version 2.x, and migrates the build process to tsdown. A critical correctness issue was identified in the multisig signing logic, where signatures are appended without being sorted in the same order as their corresponding public key hashes, which will cause on-chain verification to fail.
We didn't check if adding the last flexible signature letting us met the threshold, causing an edge case with excess flexible signatures.
Hanssen0
force-pushed
the
feat/multisig-signer
branch
from
ad03855 to
7bff329
Compare
|
@codex please review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7bff32910a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| * @returns The script args. | ||
| */ | ||
| scriptArgs(since?: SinceLike | null): Bytes { | ||
| const hash = hashCkbShort(this.toBytes()); |
There was a problem hiding this comment.
When MultisigCkbWitness already contains signatures, such as when callers pass the optional signatures field or reuse a decoded witness, this.toBytes() includes those 65-byte signatures. CKB multisig lock args are the blake160 of only the multisig script portion (S|R|M|N|PubKeyHash...), so including signatures changes the address/script args and can produce locks that do not match what the on-chain multisig script validates.
Useful? React with 👍 / 👎.
| if (pubkeyHash === thisPubkeyHash) { | ||
| // Has signed | ||
| isSignNeeded = false; | ||
| continue; |
There was a problem hiding this comment.
When the witness already contains a valid signature from this private key, signing the same transaction again marks signing as unnecessary but then skips adding that signature to the rebuilt signatures array. This makes signTransaction non-idempotent: a second call by the same signer can remove their prior signature and turn a partially or fully signed multisig transaction back into one that needs more signatures.
Useful? React with 👍 / 👎.
| await tx.addCellDepInfos(this.client, cellDeps); | ||
| return this.prepareWitnessArgsAt(tx, position); |
There was a problem hiding this comment.
When a multisig signer is constructed with options.since, the generated lock args include that timelock, but preparation only adds cell deps and a witness. Inputs collected through helpers like completeFeeBy(multisigSigner) keep the default since = 0, so spending from a timelocked multisig address fails on-chain unless every caller manually patches the matching input sinces.
Useful? React with 👍 / 👎.
| if (witness.publicKeyHashes) { | ||
| return witness.publicKeyHashes; | ||
| } | ||
| return witness.publicKeys.map((k) => hashCkbShort(k)); |
There was a problem hiding this comment.
When callers use the publicKeys form with an uncompressed 65-byte secp256k1 key, this hashes those bytes directly into the multisig script. The CKB multisig script matches signatures against the blake160 of the recovered compressed public key, so such an address can be created but its later signatures will never match the stored hash.
Useful? React with 👍 / 👎.
| ) { | ||
| super(); | ||
|
|
||
| const keysLength = publicKeyHashes.length; |
There was a problem hiding this comment.
When the same public key hash appears more than once, the on-chain multisig script treats the duplicate slots as separate entries, so a single key holder can satisfy multiple member positions by repeating the same recoverable signature. This silently weakens thresholds for accidentally duplicated publicKeys/publicKeyHashes, so the constructor should reject duplicates before deriving spendable addresses.
Useful? React with 👍 / 👎.
4 participants
This PR should be reviewed one by one according to the commits.