feat(btc): add PSBT signing and broadcasting support #346
Conversation
|
✅ Deploy Preview for docsccc 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 enhances the Bitcoin signer module by integrating robust support for Partially Signed Bitcoin Transactions (PSBTs). It establishes a standardized interface for signing and broadcasting PSBTs across various wallet integrations, allowing for both granular control over signing steps and streamlined combined operations where supported. This foundational work improves the flexibility and functionality of Bitcoin transaction handling within the system, preparing it for more complex use cases and better user experiences by reducing interaction prompts. 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 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. |
![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 adds support for signing and broadcasting Bitcoin PSBTs across various wallet integrations. The core abstraction is updated in SignerBtc, and implementations are provided for JoyID, OKX, UniSat, and Xverse. The overall approach is sound, but I have identified a couple of areas for improvement. Specifically, the design of the pushPsbt method in the base class could be clearer to avoid confusion for future implementers, and there appears to be a bug in the JoyID implementation where signing options are ignored. I've also included a minor suggestion to improve code clarity in the Xverse implementation. Please see the detailed comments below.
| /** | ||
| * Pushes a PSBT to the Bitcoin network. | ||
| * | ||
| * For wallets that support a single call for signing and broadcasting (where `supportsSingleCallSignAndBroadcast` is true), | ||
| * this method takes an **unsigned** PSBT, signs it, and broadcasts it. | ||
| * For other wallets, this method takes a **signed** PSBT and only broadcasts it. | ||
| * | ||
| * @param psbtHex - The hex string of the PSBT to push. Can be signed or unsigned depending on the wallet's capabilities. | ||
| * @param options - Options for signing the PSBT. Only used by wallets that perform signing in this step. | ||
| * @returns A promise that resolves to the transaction ID. | ||
| */ | ||
| abstract pushPsbt( | ||
| psbtHex: string, | ||
| options?: SignPsbtOptions, | ||
| ): Promise<string>; |
There was a problem hiding this comment.
The method pushPsbt has a dual responsibility depending on the supportsSingleCallSignAndBroadcast flag, which can be confusing for implementers of SignerBtc subclasses. The name pushPsbt suggests only broadcasting, but for some wallets, it also performs signing.
To improve clarity and adhere to the Single Responsibility Principle, consider splitting this into two distinct abstract methods:
broadcastPsbt(signedPsbtHex: string): Promise<string>: For broadcasting an already signed PSBT.signAndBroadcastPsbt(psbtHex: string, options?: SignPsbtOptions): Promise<string>: For wallets that support a combined sign-and-broadcast flow. You could provide a default implementation that throws an error, requiring subclasses that support it to override it.
This would make the signAndPushPsbt implementation more explicit and the contract for subclasses clearer.
There was a problem hiding this comment.
The current design reflects real Bitcoin wallet APIs where pushPsbt is standard. Wallets like Xverse/JoyID don't expose a standalone broadcast API—they only support a combined sign-and-broadcast flow—making a separate broadcastPsbt impossible to implement.
There was a problem hiding this comment.
I recommend removing the supportsSingleCallSignAndBroadcast method. As Gemini said, pushPsbt is a dual responsibility. It's unnecessary to look like the same as "standard" BTC wallet APIs, especially when it's obviously not standard.
Q: Do we have the broadcastPsbt method in some wallets? If yes, we should have a method for it and disable it (by throwing an Error) for wallets that don't support it. Then, wallets that don't support it should override the signAndPushPsbt method.
There was a problem hiding this comment.
Semantically, pushPsbt should equate to broadcastPsbt, as implemented in UniSat and OKX wallets. I'll rename pushPsbt to broadcastPsbt and override signAndPushPsbt accordingly.
packages/joy-id/src/btc/index.ts
Outdated
| async pushPsbt( | ||
| psbtHex: string, | ||
| _options?: ccc.SignPsbtOptions, | ||
| ): Promise<string> { | ||
| const { address } = await this.assertConnection(); | ||
|
|
||
| const config = this.getConfig(); | ||
| const { tx: txid } = await createPopup( | ||
| buildJoyIDURL( | ||
| { | ||
| ...config, | ||
| tx: psbtHex, | ||
| signerAddress: address, | ||
| autoFinalized: true, // sendPsbt always finalizes | ||
| isSend: true, | ||
| }, | ||
| "popup", | ||
| "/sign-psbt", | ||
| ), | ||
| { ...config, type: DappRequestType.SignPsbt }, // Use SignPsbt type for both operations | ||
| ); | ||
|
|
||
| return txid; | ||
| } |
There was a problem hiding this comment.
The pushPsbt method ignores the options parameter. If a user calls signAndPushPsbt with specific SignPsbtOptions (like toSignInputs), these options will be disregarded, which could lead to unexpected behavior or signing failures. The options should be passed to buildJoyIDURL to ensure they are respected during the sign-and-broadcast process.
async pushPsbt(
psbtHex: string,
options?: ccc.SignPsbtOptions,
): Promise<string> {
const { address } = await this.assertConnection();
const config = this.getConfig();
const { tx: txid } = await createPopup(
buildJoyIDURL(
{
...config,
tx: psbtHex,
options,
signerAddress: address,
autoFinalized: true, // sendPsbt always finalizes
isSend: true,
},
"popup",
"/sign-psbt",
),
{ ...config, type: DappRequestType.SignPsbt }, // Use SignPsbt type for both operations
);
return txid;
}There was a problem hiding this comment.
Good catch! For wallets where supportsSingleCallSignAndBroadcast is true, the options parameter should indeed be passed through to buildJoyIDURL. Fixed in 7b16831.
packages/xverse/src/signer.ts
Outdated
| // Build signInputs: Record<address, input_indexes[]> | ||
| // Multiple inputs with the same address should be grouped together |
There was a problem hiding this comment.
These comments are redundant as the prepareSignPsbtParams function already handles the logic of building signInputs. They can be removed for cleaner code.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for signing and broadcasting Bitcoin Partially Signed Bitcoin Transactions (PSBTs). It adds a new unified API (signAndPushPsbt) in the abstract SignerBtc class to handle different wallet flows (combined sign-and-broadcast vs. separate steps), which is a great approach for improving usability and avoiding multiple popups for users. The changes are well-structured, with new types for PSBT options defined in psbt.ts and implementations provided for various wallets including JoyID, OKX, UniSat, and Xverse. The use of supportsSingleCallSignAndBroadcast flag is a clean way to manage wallet-specific capabilities. I've included one suggestion to improve the robustness of PSBT hex string handling in the Xverse signer. Overall, this is a solid feature addition.
packages/xverse/src/signer.ts
Outdated
| const toSignInputs: ccc.ToSignInput[] = []; | ||
|
|
||
| try { | ||
| const psbt = Psbt.fromHex(psbtHex); |
There was a problem hiding this comment.
The psbtHex string is passed directly to Psbt.fromHex. This method from bitcoinjs-lib does not handle a "0x" prefix. To make the method more robust against different hex string formats that might be passed in, it's a good practice to sanitize the psbtHex string by removing the "0x" prefix if it exists.
| const psbt = Psbt.fromHex(psbtHex); | |
| const psbt = Psbt.fromHex(psbtHex.startsWith("0x") ? psbtHex.slice(2) : psbtHex); |
Hanssen0
left a comment
Hanssen0
left a comment
There was a problem hiding this comment.
The overall design looks good to me, but it still needs some polishing to be developer-friendly & maintainable code.
| /** | ||
| * Pushes a PSBT to the Bitcoin network. | ||
| * | ||
| * For wallets that support a single call for signing and broadcasting (where `supportsSingleCallSignAndBroadcast` is true), | ||
| * this method takes an **unsigned** PSBT, signs it, and broadcasts it. | ||
| * For other wallets, this method takes a **signed** PSBT and only broadcasts it. | ||
| * | ||
| * @param psbtHex - The hex string of the PSBT to push. Can be signed or unsigned depending on the wallet's capabilities. | ||
| * @param options - Options for signing the PSBT. Only used by wallets that perform signing in this step. | ||
| * @returns A promise that resolves to the transaction ID. | ||
| */ | ||
| abstract pushPsbt( | ||
| psbtHex: string, | ||
| options?: SignPsbtOptions, | ||
| ): Promise<string>; |
There was a problem hiding this comment.
I recommend removing the supportsSingleCallSignAndBroadcast method. As Gemini said, pushPsbt is a dual responsibility. It's unnecessary to look like the same as "standard" BTC wallet APIs, especially when it's obviously not standard.
Q: Do we have the broadcastPsbt method in some wallets? If yes, we should have a method for it and disable it (by throwing an Error) for wallets that don't support it. Then, wallets that don't support it should override the signAndPushPsbt method.
| async signAndPushPsbt( | ||
| psbtHex: string, | ||
| options?: SignPsbtOptions, | ||
| ): Promise<string> { |
There was a problem hiding this comment.
It's better to use clearer types like ccc.HexLike for the parameter and ccc.Hex for returns, so that we won't have to worry about whether a hex string is 0x-prefixed or not.
There was a problem hiding this comment.
Great suggestion! However, for return values specifically, BTC transactions default to non-0x prefixed hex. Following convention, I think it's better to return string type directly and document this in comments.
There was a problem hiding this comment.
That doesn't matter. It's because CCC uses 0x-prefixed hex strings as default. If BTC transactions return non-0x hex, we should use ccc.hexFrom to convert it.
packages/xverse/src/signer.ts
Outdated
| ).psbt; | ||
|
|
||
| const signedPsbtBytes = ccc.bytesFrom(signedPsbtBase64, "base64"); | ||
| return ccc.bytesTo(signedPsbtBytes, "hex"); // no leading "0x" |
There was a problem hiding this comment.
ccc.hexFrom is more recommended, and 0x-prefixed hex is the default choice in CCC - That's why we should use ccc.Hex type instead of string.
There was a problem hiding this comment.
Using ccc.HexLike for parameters is a great idea, but Xverse returns PSBTs as base64-encoded strings. As I understand it, ccc.hexFrom cannot directly convert base64-encoded strings.
There was a problem hiding this comment.
Sure, we should keep the bytesFrom(.., "base64"). For the Hex return type, see above.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces significant functionality by adding PSBT signing and broadcasting support across various Bitcoin signers. The changes are well-organized, with new types and abstract methods in the core package, and specific implementations in each wallet package. The use of abstract methods and default implementations in the base class is good practice. My review includes suggestions to improve code clarity and consistency, particularly around hex string manipulation and parameter naming conventions. Overall, the implementation is solid and well-thought-out.
| options?: SignPsbtOptions, | ||
| ): Promise<string> { | ||
| // ccc.hexFrom adds 0x prefix, but BTC expects non-0x | ||
| const signedPsbt = await this.signPsbt(hexFrom(psbtHex).slice(2), options); |
There was a problem hiding this comment.
The expression hexFrom(psbtHex).slice(2) is used to get a non-0x-prefixed hex string. A more direct and readable approach is to use bytesTo(bytesFrom(psbtHex), "hex"). This avoids the intermediate step of potentially adding and then immediately removing the "0x" prefix, making the intent clearer.
| const signedPsbt = await this.signPsbt(hexFrom(psbtHex).slice(2), options); | |
| const signedPsbt = await this.signPsbt(bytesTo(bytesFrom(psbtHex), "hex"), options); |
There was a problem hiding this comment.
In the signAndBroadcastPsbt flow, there are no other intermediate steps that could modify it. The current approach remains direct, clear.
There was a problem hiding this comment.
That's actually a problem. signPsbt should (also) accept a HexLike and automatically convert it. We shouldn't have to handle these type conversions here.
| throw new Error("Read-only signer does not support signPsbt"); | ||
| } | ||
|
|
||
| async broadcastPsbt(_: HexLike, __?: SignPsbtOptions): Promise<string> { |
There was a problem hiding this comment.
For consistency and readability, it's better to use named parameters prefixed with an underscore (e.g., _psbtHex) to indicate they are unused, rather than _ and __. This aligns with the convention used in other parts of the codebase.
| async broadcastPsbt(_: HexLike, __?: SignPsbtOptions): Promise<string> { | |
| async broadcastPsbt(_psbtHex: HexLike, _options?: SignPsbtOptions): Promise<string> { |
| buildJoyIDURL( | ||
| { | ||
| ...config, | ||
| tx: ccc.hexFrom(psbtHex).slice(2), |
There was a problem hiding this comment.
The expression ccc.hexFrom(psbtHex).slice(2) is used to get a non-0x-prefixed hex string. A more direct and readable approach is to use ccc.bytesTo(ccc.bytesFrom(psbtHex), "hex"). This avoids the intermediate step of potentially adding and then immediately removing the "0x" prefix.
| tx: ccc.hexFrom(psbtHex).slice(2), | |
| tx: ccc.bytesTo(ccc.bytesFrom(psbtHex), "hex"), |
| buildJoyIDURL( | ||
| { | ||
| ...config, | ||
| tx: ccc.hexFrom(psbtHex).slice(2), |
There was a problem hiding this comment.
The expression ccc.hexFrom(psbtHex).slice(2) is used to get a non-0x-prefixed hex string. A more direct and readable approach is to use ccc.bytesTo(ccc.bytesFrom(psbtHex), "hex"). This avoids the intermediate step of potentially adding and then immediately removing the "0x" prefix.
| tx: ccc.hexFrom(psbtHex).slice(2), | |
| tx: ccc.bytesTo(ccc.bytesFrom(psbtHex), "hex"), |
packages/okx/src/btc/index.ts
Outdated
| psbtHex: ccc.HexLike, | ||
| options?: ccc.SignPsbtOptions, | ||
| ): Promise<string> { | ||
| return this.provider.signPsbt(ccc.hexFrom(psbtHex).slice(2), options); |
There was a problem hiding this comment.
The expression ccc.hexFrom(psbtHex).slice(2) is used to get a non-0x-prefixed hex string. A more direct and readable approach is to use ccc.bytesTo(ccc.bytesFrom(psbtHex), "hex"). This avoids the intermediate step of potentially adding and then immediately removing the "0x" prefix.
| return this.provider.signPsbt(ccc.hexFrom(psbtHex).slice(2), options); | |
| return this.provider.signPsbt(ccc.bytesTo(ccc.bytesFrom(psbtHex), "hex"), options); |
packages/okx/src/btc/index.ts
Outdated
| psbtHex: ccc.HexLike, | ||
| _options?: ccc.SignPsbtOptions, | ||
| ): Promise<string> { | ||
| return this.provider.pushPsbt(ccc.hexFrom(psbtHex).slice(2)); |
There was a problem hiding this comment.
The expression ccc.hexFrom(psbtHex).slice(2) is used to get a non-0x-prefixed hex string. A more direct and readable approach is to use ccc.bytesTo(ccc.bytesFrom(psbtHex), "hex"). This avoids the intermediate step of potentially adding and then immediately removing the "0x" prefix.
| return this.provider.pushPsbt(ccc.hexFrom(psbtHex).slice(2)); | |
| return this.provider.pushPsbt(ccc.bytesTo(ccc.bytesFrom(psbtHex), "hex")); |
packages/uni-sat/src/signer.ts
Outdated
| psbtHex: ccc.HexLike, | ||
| options?: ccc.SignPsbtOptions, | ||
| ): Promise<string> { | ||
| return this.provider.signPsbt(ccc.hexFrom(psbtHex).slice(2), options); |
There was a problem hiding this comment.
The expression ccc.hexFrom(psbtHex).slice(2) is used to get a non-0x-prefixed hex string. A more direct and readable approach is to use ccc.bytesTo(ccc.bytesFrom(psbtHex), "hex"). This avoids the intermediate step of potentially adding and then immediately removing the "0x" prefix.
| return this.provider.signPsbt(ccc.hexFrom(psbtHex).slice(2), options); | |
| return this.provider.signPsbt(ccc.bytesTo(ccc.bytesFrom(psbtHex), "hex"), options); |
packages/uni-sat/src/signer.ts
Outdated
| psbtHex: ccc.HexLike, | ||
| _options?: ccc.SignPsbtOptions, | ||
| ): Promise<string> { | ||
| return this.provider.pushPsbt(ccc.hexFrom(psbtHex).slice(2)); |
There was a problem hiding this comment.
The expression ccc.hexFrom(psbtHex).slice(2) is used to get a non-0x-prefixed hex string. A more direct and readable approach is to use ccc.bytesTo(ccc.bytesFrom(psbtHex), "hex"). This avoids the intermediate step of potentially adding and then immediately removing the "0x" prefix.
| return this.provider.pushPsbt(ccc.hexFrom(psbtHex).slice(2)); | |
| return this.provider.pushPsbt(ccc.bytesTo(ccc.bytesFrom(psbtHex), "hex")); |
| _: ccc.HexLike, | ||
| __?: ccc.SignPsbtOptions, |
There was a problem hiding this comment.
The parameter names _ and __ are obscure and inconsistent with signPsbt in the same file, which uses _psbtHex and _options. For better readability and consistency, it's better to use named parameters prefixed with an underscore to indicate they are unused.
| _: ccc.HexLike, | |
| __?: ccc.SignPsbtOptions, | |
| _psbtHex: ccc.HexLike, | |
| _options?: ccc.SignPsbtOptions, |
2 participants
No description provided.