Network-Aware JWT Authentication and BTC Address Balance Query Interface #240
Conversation
✅ 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. |
✅ Deploy Preview for liveccc 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.
Summary of Changes
Hello @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 (BTC) integration within the system by introducing a standardized interface for Partially Signed Bitcoin Transaction (PSBT) operations, including signing and broadcasting. It also adds a network-aware mechanism for JWT authentication with the BTC asset API and a new interface for querying BTC address balances. These changes lay the groundwork for more robust and flexible Bitcoin-related functionalities, particularly for RGB++ operations, and are demonstrated through a new UI feature in the demo application.
Highlights
- Standardized BTC PSBT Operations: Introduced abstract
signPsbtandpushPsbtmethods inSignerBtc(core package) to provide a consistent API for Bitcoin signers to handle Partially Signed Bitcoin Transactions (PSBTs). - Wallet Integration for PSBTs: Implemented concrete
signPsbtandpushPsbtmethods for JoyID, OKX, and UniSat signers, enabling these wallets to sign and broadcast Bitcoin transactions. Placeholder implementations were added for UTXO Global and Xverse. - Network-Aware BTC Asset API Authentication: The
BtcAssetsApiBasenow conditionally requires JWT authentication (token and origin) based on whether the network is mainnet, improving security and flexibility for API interactions. - BTC Address Balance Query: Added a new
getBalancemethod toRgbppBtcWalletand associated types (BtcApiBalance,BtcApiBalanceParams) to allow querying Bitcoin address balances from the BTC asset API. - Refactored BTC Wallet Abstraction: The
RgbppBtcWalletclass was refactored into an abstract class, separating the core wallet logic from specific private key or browser-based implementations. NewBrowserRgbppBtcWalletandPrivateKeyRgbppBtcWalletconcrete classes were introduced. - Improved Fee Estimation: The
estimateFeelogic inRgbppBtcWalletwas enhanced to accurately estimate transaction virtual size and fees without requiring actual wallet signing, preventing unnecessary wallet prompts. - New RGB++ xUDT Issuance Demo: A new UI page (
IssueRgbppXUdt/page.tsx) was added to the demo application, showcasing the end-to-end process of issuing RGB++ xUDT, leveraging the new BTC wallet and PSBT capabilities. - Examples Refactoring: The example scripts for RGB++ operations (Spore and UDT) were refactored to use the new
initializeRgbppEnvandload-envutilities, improving modularity and consistency.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
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 or fill out our survey to provide feedback.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
✅ Deploy Preview for appccc ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Code Review
This pull request introduces significant new functionality for handling RGB++ assets, including a new demo page for issuing xUDT. It refactors the RgbppBtcWallet into an abstract class with concrete implementations for private key and browser-based wallets, which is a great design improvement. It also adds PSBT signing and broadcasting capabilities to several wallet connectors (JoyID, OKX, UniSat).
The review identifies a critical compilation error due to a duplicate method declaration, a few high-severity issues related to unsafe environment variable access and an interface contract violation, and several medium-severity issues concerning code clarity, maintainability, and robustness. These include the use of magic numbers, brittle wallet detection logic, and hardcoded values that should be dynamic. Addressing these points will improve the overall quality and stability of the new features.
| // TODO: tweaked signer for taproot | ||
| signPsbt(psbtHex: string): Promise<string>; | ||
|
|
||
| pushPsbt(psbtHex: string): Promise<string>; | ||
|
|
||
| pushTx(tx: { rawtx: string }): Promise<string>; |
There was a problem hiding this comment.
The methods signPsbt and pushPsbt are declared twice in the Provider interface. This will cause a TypeScript compilation error. Please remove the duplicate declarations that lack JSDoc comments.
| // TODO: tweaked signer for taproot | |
| signPsbt(psbtHex: string): Promise<string>; | |
| pushPsbt(psbtHex: string): Promise<string>; | |
| pushTx(tx: { rawtx: string }): Promise<string>; | |
| pushTx(tx: { rawtx: string }): Promise<string>; |
| } | ||
|
|
||
| const config: BtcAssetApiConfig = { | ||
| url: process.env.NEXT_PUBLIC_BTC_ASSETS_API_URL!, |
There was a problem hiding this comment.
Using a non-null assertion (!) on process.env.NEXT_PUBLIC_BTC_ASSETS_API_URL can lead to a runtime crash if the environment variable is not set. It's safer to validate its existence and provide a clear error message. This will improve the robustness and debuggability of the application.
const btcAssetsApiUrl = process.env.NEXT_PUBLIC_BTC_ASSETS_API_URL;
if (!btcAssetsApiUrl) {
error("Configuration error: NEXT_PUBLIC_BTC_ASSETS_API_URL is not set.");
return null;
}
const config: BtcAssetApiConfig = {
url: btcAssetsApiUrl,
// ...| async pushPsbt(psbtHex: string): 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 base SignerBtc.pushPsbt method is documented to accept a signed PSBT for broadcasting. However, this implementation of pushPsbt for JoyID accepts an unsigned PSBT and performs both signing and broadcasting. This violates the interface contract of the superclass and the Liskov Substitution Principle.
While BrowserRgbppBtcWallet correctly handles this special case, this inconsistency can lead to bugs if other parts of the application use SignerBtc polymorphically.
Consider renaming this method to something like signAndPushPsbt to make its behavior explicit and align with its functionality.
| only_non_rgbpp_utxos: true, | ||
| }); | ||
|
|
||
| await new Promise((resolve) => setTimeout(resolve, 800)); |
There was a problem hiding this comment.
This line introduces an artificial delay of 800ms. If this is intended for UX purposes (e.g., to ensure a loading spinner is visible for a minimum duration), it would be helpful to add a comment explaining why. Otherwise, it should be removed to avoid unnecessarily slowing down the UTXO fetching process. This same pattern is also present on line 397.
// The following line introduces an artificial delay for UX purposes.
await new Promise((resolve) => setTimeout(resolve, 800));| rgbppUdtClient, | ||
| btcChangeAddress: btcAccount, | ||
| receiverBtcAddresses: [btcAccount], | ||
| feeRate: 10, |
There was a problem hiding this comment.
The Bitcoin transaction fee rate is hardcoded to 10. This might not be optimal, as network conditions change. Consider fetching a recommended fee rate from an API (like the one RgbppBtcWallet provides with getRecommendedFee) or allowing the user to set it. This would make the transaction more robust and cost-effective.
| if ( | ||
| this.signer.constructor.name === "BitcoinSigner" && | ||
| "name" in this.signer | ||
| ) { |
There was a problem hiding this comment.
The method used to identify the JoyID signer by checking this.signer.constructor.name and property existence ("name" in this.signer) is brittle. It can break if the underlying JoyID library changes its implementation details (e.g., renames the class or property).
A more robust approach would be to use a unique identifier or type property on the signer object itself, or use instanceof with specific signer classes if they are available. While this might be the only option given the current wallet provider implementations, it's a potential point of failure in the future. The same applies to the check in createBrowserRgbppBtcWallet on lines 43-46.
| * Estimate virtual size of a transaction | ||
| * Based on Bitcoin transaction structure and different address types | ||
| */ | ||
| private estimateVirtualSize( |
There was a problem hiding this comment.
The size calculations within this switch statement use magic numbers (e.g., 72, 33, 64). This makes the logic difficult to understand and verify. It's recommended to define these values as named constants to improve code clarity and maintainability.
const ECDSA_SIGNATURE_SIZE = 72;
const COMPRESSED_PUBKEY_SIZE = 33;
const SCHNORR_SIGNATURE_SIZE = 64;
// ... inside the switch case
case "P2WPKH":
// ...
witnessSize += 1 + 1 + ECDSA_SIGNATURE_SIZE + 1 + COMPRESSED_PUBKEY_SIZE;
break;
|
This PR seems to include lots of things. Do we have a recommended order to review these changes? |
I accidentally developed on the wrong branch. Only the latest two commits are the intended changes. I’ll close this PR and resubmit. |
2 participants
No description provided.