feat(btc): add PSBT signing and broadcasting support #334
Conversation
|
✅ Deploy Preview for apiccc ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
❌ Deploy Preview for appccc failed. Why did it fail? →
|
❌ Deploy Preview for liveccc failed. Why did it fail? →
|
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 capabilities by integrating support for Partially Signed Bitcoin Transactions (PSBT). It establishes a standardized interface for signing and broadcasting PSBTs within the core library and extends this functionality to various wallet integrations, including JoyID, OKX, UniSat, and Xverse. The changes aim to provide a more robust and user-friendly experience for handling complex Bitcoin transactions, particularly by enabling a combined sign-and-broadcast operation for compatible wallets to minimize user prompts. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
✅ Deploy Preview for docsccc ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for signing and broadcasting Partially Signed Bitcoin Transactions (PSBTs) across various wallet signers. It adds a new abstraction layer in @ckb-ccc/core with signPsbt and pushPsbt methods, and a signAndPushPsbt helper to handle different wallet flows (combined vs. separate sign/broadcast). Implementations are provided for JoyID, OKX, UniSat, and Xverse wallets.
The changes are well-structured. I've identified a few areas for improvement:
- The documentation for the new
pushPsbtmethod in the baseSignerBtcclass could be clearer about its dual purpose. - The Xverse signer implementation has some fallback logic that could be risky. Specifically, it attempts to re-sign already signed transactions and swallows PSBT parsing errors.
Overall, this is a great feature addition. Addressing the points above will improve the robustness and maintainability of the new functionality.
| /** | ||
| * Broadcasts a signed PSBT to the Bitcoin network. | ||
| * | ||
| * @param psbtHex - The hex string of signed PSBT to broadcast | ||
| * @returns A promise that resolves to the transaction ID | ||
| */ |
There was a problem hiding this comment.
The documentation for pushPsbt is a bit misleading. For wallets that support a single call for signing and broadcasting (supportsSingleCallSignAndBroadcast is true), this method actually receives an unsigned PSBT and is responsible for both signing and broadcasting. The current documentation only describes the behavior for wallets that separate signing and broadcasting. This can cause confusion for developers implementing new signers.
Please update the documentation to clarify the dual role of this method depending on the wallet's capabilities.
/**
* 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.
*/
packages/xverse/src/signer.ts
Outdated
| if (toSignInputs.length === 0) { | ||
| for (let i = 0; i < psbt.data.inputs.length; i++) { | ||
| toSignInputs.push({ index: i, address } as ccc.ToSignInput); | ||
| } | ||
| } |
There was a problem hiding this comment.
This logic block, which triggers when no unsigned inputs are found, re-populates toSignInputs with all inputs. This could lead to re-signing an already signed transaction, which is likely not the desired behavior and may cause unnecessary signing requests or errors. It's safer to let toSignInputs remain empty. The wallet will then likely perform a no-op, which is the correct behavior for an already signed PSBT.
| } catch (error) { | ||
| // Fallback: if PSBT parsing fails, assume single input at index 0 | ||
| console.warn("Failed to parse PSBT, assuming single input:", error); | ||
| toSignInputs.push({ index: 0, address } as ccc.ToSignInput); | ||
| } |
There was a problem hiding this comment.
The try...catch block here swallows a potentially important error from Psbt.fromHex. If the PSBT hex is invalid, parsing will fail. Instead of catching the error and making an assumption to sign input 0, it would be safer to let the error propagate up to the caller. This would make debugging easier and prevent unexpected signing behavior on invalid PSBTs. The current implementation could mask an underlying issue with the provided PSBT.
| } catch (error) { | |
| // Fallback: if PSBT parsing fails, assume single input at index 0 | |
| console.warn("Failed to parse PSBT, assuming single input:", error); | |
| toSignInputs.push({ index: 0, address } as ccc.ToSignInput); | |
| } | |
| } catch (error) { | |
| // Re-throw the error to the caller for proper handling. | |
| // Assuming input 0 could lead to unexpected behavior with an invalid PSBT. | |
| console.error("Failed to parse PSBT:", error); | |
| throw error; | |
| } |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces PSBT signing and broadcasting capabilities, which is a significant and well-structured feature. The changes include a base abstract class SignerBtc defining the new interface, with implementations for various wallets like JoyID, OKX, UniSat, and Xverse. The design thoughtfully handles wallets with combined sign-and-broadcast flows as well as those with separate steps.
My review focuses on improving type consistency and documentation. I've suggested using the Hex type for hexadecimal string return values across the new methods to enhance type safety and align with existing project conventions. Additionally, I've pointed out a few areas where documentation could be improved for better clarity and maintainability.
| async signAndPushPsbt( | ||
| psbtHex: string, | ||
| options?: SignPsbtOptions, | ||
| ): Promise<string> { | ||
| if (this.supportsSingleCallSignAndBroadcast) { | ||
| // Wallet handles sign+broadcast internally (e.g., Xverse/JoyID) | ||
| return this.pushPsbt(psbtHex, options); | ||
| } | ||
|
|
||
| // Split-mode wallets: sign first, then broadcast | ||
| const signedPsbt = await this.signPsbt(psbtHex, options); | ||
| return this.pushPsbt(signedPsbt, options); | ||
| } |
There was a problem hiding this comment.
To improve type safety and consistency across the codebase, I recommend changing the return types of signAndPushPsbt, signPsbt, and pushPsbt from Promise<string> to Promise<Hex>.
The Hex type ensures that hexadecimal strings are 0x-prefixed, which is a convention used elsewhere in the project (e.g., hexFrom). This change would make the interfaces more explicit about the expected format of return values (signed PSBT hex and transaction ID).
For example, signPsbt would become:
abstract signPsbt(
psbtHex: string,
options?: SignPsbtOptions,
): Promise<Hex>;This would require updating the implementing classes. For instance, in the Xverse signer, you could then remove .slice(2) from the return statement in signPsbt, making the code cleaner and less prone to errors.
There was a problem hiding this comment.
Bitcoin tx IDs and PSBTs are bare hex strings with no 0x prefix by Bitcoin ecosystem convention.
| pushPsbt(psbtHex: string): Promise<string>; | ||
|
|
||
| pushTx(tx: { rawtx: string }): Promise<string>; |
There was a problem hiding this comment.
The new methods pushPsbt and pushTx are missing JSDoc comments. Adding documentation would improve clarity on their purpose, parameters, and return values for other developers using this interface.
For example:
/**
* Broadcasts a signed PSBT to the Bitcoin network.
*
* @param psbtHex - The hex string of the signed PSBT to broadcast.
* @returns A promise that resolves to the transaction ID.
*/
pushPsbt(psbtHex: string): Promise<string>;
/**
* Pushes a raw transaction to the Bitcoin network.
*
* @param tx - An object containing the raw transaction hex.
* @returns A promise that resolves to the transaction ID.
*/
pushTx(tx: { rawtx: string }): Promise<string>;| * Signs a PSBT using UniSat wallet. | ||
| * | ||
| * @param psbtHex - The hex string of PSBT to sign | ||
| * @returns A promise that resolves to the signed PSBT hex string | ||
| */ |
There was a problem hiding this comment.
The JSDoc for signPsbt is missing the @param tag for the options parameter. Please add it for completeness and to improve documentation clarity.
| * Signs a PSBT using UniSat wallet. | |
| * | |
| * @param psbtHex - The hex string of PSBT to sign | |
| * @returns A promise that resolves to the signed PSBT hex string | |
| */ | |
| * @param psbtHex - The hex string of PSBT to sign | |
| * @param options - Options for signing the PSBT | |
| * @returns A promise that resolves to the signed PSBT hex string |
packages/xverse/src/signer.ts
Outdated
| (input.tapScriptSig && input.tapScriptSig.length > 0); | ||
|
|
||
| if (!isSigned) { | ||
| toSignInputs.push({ index, address } as ccc.ToSignInput); |
There was a problem hiding this comment.
The type cast as ccc.ToSignInput appears to be redundant here. The object literal { index, address } already satisfies the ccc.ToSignInput type, as it matches the { address: string; publicKey?: string; } part of the union. You can safely remove the cast to make the code cleaner.
| toSignInputs.push({ index, address } as ccc.ToSignInput); | |
| toSignInputs.push({ index, address }); |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces robust support for signing and broadcasting Bitcoin Partially Signed Bitcoin Transactions (PSBTs), which is a significant and well-implemented feature. The abstraction layer in SignerBtc is thoughtfully designed to accommodate different wallet behaviors, distinguishing between those that combine signing and broadcasting and those that handle them as separate steps. The implementations for various wallets like JoyID, OKX, UniSat, and Xverse correctly adhere to this new abstraction. I have a couple of minor suggestions to improve type definitions and implementation details for better code clarity and robustness. Overall, this is a solid contribution.
| } & ( | ||
| | { | ||
| /** | ||
| * The address whose corresponding private key to use for signing. | ||
| */ | ||
| address: string; | ||
| /** | ||
| * The public key whose corresponding private key to use for signing. | ||
| */ | ||
| publicKey?: string; | ||
| } | ||
| | { | ||
| /** | ||
| * The address whose corresponding private key to use for signing. | ||
| */ | ||
| address?: string; | ||
| /** | ||
| * The public key whose corresponding private key to use for signing. | ||
| */ | ||
| publicKey: string; | ||
| } | ||
| ); |
There was a problem hiding this comment.
The ToSignInput type has duplicated properties and comments for address and publicKey. This can be refactored to improve readability and maintainability by defining the properties once and then expressing the constraint that at least one of them must be present.
} & {
/**
* The address whose corresponding private key to use for signing.
*/
address?: string;
/**
* The public key whose corresponding private key to use for signing.
*/
publicKey?: string;
} & ({ address: string } | { publicKey: string });
packages/xverse/src/signer.ts
Outdated
| const signedPsbtBytes = ccc.bytesFrom(signedPsbtBase64, "base64"); | ||
| return ccc.hexFrom(signedPsbtBytes).slice(2); |
There was a problem hiding this comment.
Using ccc.hexFrom(...).slice(2) to remove the '0x' prefix is a bit indirect and relies on the prefix being '0x'. It's cleaner and more robust to use ccc.bytesTo(..., 'hex') which directly converts bytes to a hex string without the prefix.
| const signedPsbtBytes = ccc.bytesFrom(signedPsbtBase64, "base64"); | |
| return ccc.hexFrom(signedPsbtBytes).slice(2); | |
| const signedPsbtBytes = ccc.bytesFrom(signedPsbtBase64, "base64"); | |
| return ccc.bytesTo(signedPsbtBytes, "hex"); |
1 participant
No description provided.