-
Notifications
You must be signed in to change notification settings - Fork 28
feat(bitcoin): memo (OP_RETURN) based transactions #332
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
📝 WalkthroughWalkthroughThe changes introduce a modular redesign of the Bitcoin CLI commands for ZetaChain, splitting functionality into "inscription" and "memo" command groups. Legacy deposit, call, and encode commands are restructured and extended with new schemas, fee calculations, and transaction helpers. Memo-based transaction support is added, and all commands now use updated validation, fee, and broadcast flows. Changes
Sequence Diagram(s)Inscription Deposit FlowsequenceDiagram
participant User
participant CLI
participant BitcoinHelpers
participant BitcoinAPI
User->>CLI: Run "inscription deposit" command with options
CLI->>BitcoinHelpers: Setup Bitcoin key from private key or name
CLI->>BitcoinAPI: Fetch UTXOs for sender address
CLI->>BitcoinHelpers: Prepare deposit data (receiver/revert or raw)
CLI->>BitcoinHelpers: Create commit transaction
CLI->>BitcoinHelpers: Calculate reveal and deposit fees
CLI->>User: Display transaction details and confirm
User->>CLI: Confirm transaction
CLI->>BitcoinAPI: Broadcast commit transaction
CLI->>BitcoinHelpers: Create reveal transaction (using commit TXID)
CLI->>BitcoinAPI: Broadcast reveal transaction
CLI->>User: Output commit and reveal TXIDs
Memo Deposit FlowsequenceDiagram
participant User
participant CLI
participant BitcoinHelpers
participant BitcoinAPI
User->>CLI: Run "memo deposit" command with options
CLI->>BitcoinHelpers: Setup Bitcoin key from private key or name
CLI->>BitcoinAPI: Fetch UTXOs for sender address
CLI->>BitcoinHelpers: Prepare memo from receiver address (and data)
CLI->>BitcoinHelpers: Calculate deposit and network fees
CLI->>User: Display transaction details and confirm
User->>CLI: Confirm transaction
CLI->>BitcoinHelpers: Create transaction with memo (OP_RETURN)
CLI->>BitcoinAPI: Broadcast transaction
CLI->>User: Output transaction hash
Command Group Structuregraph TD
bitcoinCommand --> inscriptionCommand
bitcoinCommand --> memoCommand
inscriptionCommand --> callCommand
inscriptionCommand --> depositAndCallCommand
inscriptionCommand --> depositCommand
inscriptionCommand --> encodeCommand
memoCommand --> callCommand_memo
memoCommand --> depositAndCallCommand_memo
memoCommand --> depositCommand_memo
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (6)
🧰 Additional context used🧬 Code Graph Analysis (1)utils/bitcoin.helpers.ts (3)
🔇 Additional comments (7)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Currently, the problem is the amount we're submitting is below the deposit fee. Need to implement logic to calculate it. |
Added deposit fee calculation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 7
🔭 Outside diff range comments (1)
utils/bitcoin.helpers.ts (1)
253-275
: 🛠️ Refactor suggestionAvoid duplicated vsize math & replace “magic” numbers with documented constants
calculateFees
repeats the same vsize expression twice (once forrevealFee
, again fortxVsize
). This bloats the function and is easy to get out of sync if constants change.
In addition, the68 * 2
factor incalculateDepositFee
is undocumented; the intent (two P2WPKH outputs?) is not obvious and should be expressed with descriptive constants (BITCOIN_TX.P2WPKH_OUTPUT_VBYTES * 2
) or at least an inline comment.Suggested refactor:
- const revealFee = Math.ceil( - (BITCOIN_TX.TX_OVERHEAD + - 36 + - 1 + - 43 + - Math.ceil(data.length / 4) + - BITCOIN_TX.P2WPKH_OUTPUT_VBYTES) * - BITCOIN_FEES.DEFAULT_REVEAL_FEE_RATE - ); - - const txVsize = - BITCOIN_TX.TX_OVERHEAD + - 36 + - 1 + - 43 + - Math.ceil(data.length / 4) + - BITCOIN_TX.P2WPKH_OUTPUT_VBYTES; + const inputVsize = + 36 + // outpoint & sequence + 1 + // script-length marker + 43; // segwit marker/flag & witness weight + const txVsize = + BITCOIN_TX.TX_OVERHEAD + + inputVsize + + BITCOIN_TX.P2WPKH_OUTPUT_VBYTES + + Math.ceil(data.length / 4); + + const revealFee = Math.ceil(txVsize * BITCOIN_FEES.DEFAULT_REVEAL_FEE_RATE);-export const calculateDepositFee = (txFee: number, txVsize: number): number => { - return Math.ceil((txFee / txVsize) * 68 * 2); +// 2 × P2WPKH outputs (change + gateway) +const P2WPKH_PAIR_VBYTES = BITCOIN_TX.P2WPKH_OUTPUT_VBYTES * 2; + +export const calculateDepositFee = (txFee: number, txVsize: number): number => + Math.ceil((txFee / txVsize) * P2WPKH_PAIR_VBYTES);This removes duplication, clarifies the 68-byte constant, and future-proofs the calculation.
🧹 Nitpick comments (6)
types/bitcoin.types.ts (1)
55-56
: ExposebitcoinMethods
to consumers & testsGreat to see the union captured in a single constant. Consider exporting it (or a derived
BitcoinMethod
union type) so other modules and unit tests can reuse the source of truth instead of duplicating"inscription" | "memo"
literals.
This reduces drift if another method is added later.packages/commands/src/bitcoin/call.ts (1)
74-88
:encodingFormat: "ABI"
may mislead when raw--data
is suppliedWhen the user supplies raw
--data
,payload
isundefined
, yetencodingFormat
is still hard-coded to"ABI"
.
Either detect the format or change the label to “Raw” whenpayload
is falsy to avoid confusing confirmations.packages/commands/src/bitcoin/deposit.ts (1)
90-98
: Use same validation guard here as suggested in call.tsReplication of the
--data
check in memo mode keeps UX consistent:if (!options.data) { throw new Error("The --data flag is required when --method memo is used"); }utils/bitcoinMemo.helpers.ts (2)
63-66
: Sequential HTTP calls hurt performanceFetching each parent tx inside a
for
loop blocks on network latency N times. UsePromise.all
to parallelise:-const txs = []; -for (const utxo of pickUtxos) { - const { data } = await axios.get<BtcTxById>(`${api}/tx/${utxo.txid}`); - txs.push(data); -} +const txs: BtcTxById[] = await Promise.all( + pickUtxos.map((u) => axios.get<BtcTxById>(`${api}/tx/${u.txid}`).then(r => r.data)) +);
87-93
: Redundant assignment
inputData.witnessUtxo = witnessUtxo;
duplicates the previously-set property; the extra line can be removed to simplify the code.utils/bitcoin.command.helpers.ts (1)
130-132
: Validate hex & accept0x
-prefixed memo
Buffer.from(memo, "hex")
will crash on odd-length strings (common when users forget a leading 0) and rejects0x
prefixes. Consider:-const memoBuffer = Buffer.from(memo, "hex"); +import { trimOx } from "./bitcoinEncode"; +const hex = trimOx(memo); +if (!/^[0-9a-fA-F]*$/.test(hex) || hex.length % 2 !== 0) { + throw new Error("Memo must be valid hex"); +} +const memoBuffer = Buffer.from(hex, "hex");This safeguards the confirmation step and keeps UX consistent with other helpers.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/commands/src/bitcoin/call.ts
(3 hunks)packages/commands/src/bitcoin/deposit.ts
(3 hunks)packages/commands/src/bitcoin/depositAndCall.ts
(3 hunks)types/bitcoin.types.ts
(3 hunks)utils/bitcoin.command.helpers.ts
(4 hunks)utils/bitcoin.helpers.ts
(2 hunks)utils/bitcoinMemo.helpers.ts
(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (7)
types/bitcoin.types.ts (1)
types/shared.schema.ts (2)
validAmountSchema
(28-32)hexStringSchema
(38-40)
packages/commands/src/bitcoin/deposit.ts (5)
utils/bitcoinEncode.ts (1)
bitcoinEncode
(57-71)types/bitcoin.constants.ts (1)
BITCOIN_LIMITS
(40-48)utils/bitcoin.helpers.ts (1)
calculateFees
(251-275)utils/bitcoin.command.helpers.ts (4)
displayAndConfirmTransaction
(96-119)createAndBroadcastTransactions
(163-203)displayAndConfirmMemoTransaction
(124-144)broadcastBtcTransaction
(149-158)utils/bitcoinMemo.helpers.ts (1)
bitcoinMakeTransactionWithMemo
(30-102)
packages/commands/src/bitcoin/depositAndCall.ts (5)
utils/bitcoinEncode.ts (2)
bitcoinEncode
(57-71)trimOx
(183-185)types/bitcoin.constants.ts (1)
BITCOIN_LIMITS
(40-48)utils/bitcoin.helpers.ts (1)
calculateFees
(251-275)utils/bitcoin.command.helpers.ts (3)
displayAndConfirmTransaction
(96-119)createAndBroadcastTransactions
(163-203)broadcastBtcTransaction
(149-158)utils/bitcoinMemo.helpers.ts (1)
bitcoinMakeTransactionWithMemo
(30-102)
utils/bitcoinMemo.helpers.ts (2)
types/bitcoin.constants.ts (2)
BITCOIN_TX
(28-35)BITCOIN_FEES
(53-57)types/bitcoin.types.ts (2)
BtcUtxo
(11-21)BtcTxById
(38-53)
utils/bitcoin.helpers.ts (1)
types/bitcoin.constants.ts (1)
BITCOIN_TX
(28-35)
utils/bitcoin.command.helpers.ts (2)
utils/bitcoinMemo.helpers.ts (1)
calculateMemoTransactionFee
(18-28)types/bitcoin.types.ts (1)
bitcoinMethods
(55-55)
packages/commands/src/bitcoin/call.ts (5)
utils/bitcoinEncode.ts (2)
bitcoinEncode
(57-71)trimOx
(183-185)types/bitcoin.constants.ts (1)
BITCOIN_LIMITS
(40-48)utils/bitcoin.helpers.ts (1)
calculateFees
(251-275)utils/bitcoin.command.helpers.ts (4)
displayAndConfirmTransaction
(96-119)createAndBroadcastTransactions
(163-203)displayAndConfirmMemoTransaction
(124-144)broadcastBtcTransaction
(149-158)utils/bitcoinMemo.helpers.ts (1)
bitcoinMakeTransactionWithMemo
(30-102)
const amount = | ||
BITCOIN_LIMITS.MIN_COMMIT_AMOUNT + BITCOIN_LIMITS.ESTIMATED_REVEAL_FEE; | ||
|
||
const amount = | ||
BITCOIN_LIMITS.MIN_COMMIT_AMOUNT + BITCOIN_LIMITS.ESTIMATED_REVEAL_FEE; | ||
const { commitFee, revealFee, depositFee, totalFee } = calculateFees(data); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deposited satoshis ignore --amount
for inscription method
amount
is currently hard-coded to MIN_COMMIT_AMOUNT + ESTIMATED_REVEAL_FEE
(~5.5 k sat) and the real --amount
from the CLI is only echoed to the user.
The actual transaction therefore deposits the minimum amount regardless of what the user requested—likely a critical bug.
-const amount =
- BITCOIN_LIMITS.MIN_COMMIT_AMOUNT + BITCOIN_LIMITS.ESTIMATED_REVEAL_FEE;
+const amount = Number(ethers.parseUnits(options.amount, 8));
+if (amount < BITCOIN_LIMITS.MIN_COMMIT_AMOUNT + BITCOIN_LIMITS.ESTIMATED_REVEAL_FEE) {
+ throw new Error(
+ `Amount must be at least ${
+ (BITCOIN_LIMITS.MIN_COMMIT_AMOUNT + BITCOIN_LIMITS.ESTIMATED_REVEAL_FEE) / 1e8
+ } BTC to cover commit & reveal`
+ );
+}
Without this fix users will believe they’ve deposited (say) 0.1 BTC while only ~0.000055 BTC is actually sent.
🤖 Prompt for AI Agents
In packages/commands/src/bitcoin/deposit.ts around lines 55 to 58, the variable
`amount` is incorrectly hard-coded to the sum of `MIN_COMMIT_AMOUNT` and
`ESTIMATED_REVEAL_FEE`, ignoring the user-supplied `--amount` from the CLI. To
fix this, replace the hard-coded value with the actual amount parsed from the
CLI input, ensuring the deposit transaction uses the correct user-specified
amount rather than the minimum default.
utils/bitcoinMemo.helpers.ts
Outdated
const memo = Buffer.from(m, "hex"); | ||
console.log(memo.length); | ||
|
||
if (!memo || memo.length < 20) throw new Error(errorNoReceiver); | ||
if (memo.length > 80) throw new Error(errorTooLong); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Unsafe Buffer.from(..., "hex")
– add validation
Buffer.from(m, "hex")
throws for odd-length/invalid hex and the error is opaque. Validate and trim an optional 0x
prefix before converting, surfacing a clear message to the user.
-const memo = Buffer.from(m, "hex");
+const hex = trimOx(m);
+if (!/^[0-9a-fA-F]*$/.test(hex) || hex.length % 2 !== 0) {
+ throw new Error("Memo must be a valid hex string");
+}
+const memo = Buffer.from(hex, "hex");
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In utils/bitcoinMemo.helpers.ts around lines 40 to 45, the code uses
Buffer.from(m, "hex") without validating the input, which can throw an opaque
error for invalid or odd-length hex strings. To fix this, first check if the
string starts with "0x" and remove it, then validate that the remaining string
is a valid even-length hex string before calling Buffer.from. If validation
fails, throw a clear, descriptive error message to the user instead of letting
Buffer.from throw.
utils/bitcoinMemo.helpers.ts
Outdated
export const calculateMemoTransactionFee = (memoLength: number): number => { | ||
// Calculate transaction size in vbytes | ||
const txOverhead = BITCOIN_TX.TX_OVERHEAD; | ||
const inputVbytes = 36 + 1 + 43 + 107; // txin + marker+flag + varint scriptSig len (0) + sequence + witness weight/4 | ||
const outputVbytes = BITCOIN_TX.P2WPKH_OUTPUT_VBYTES; | ||
const memoOutputVbytes = 9 + memoLength; // varint len + memo length | ||
const vsize = txOverhead + inputVbytes + outputVbytes + memoOutputVbytes; | ||
|
||
// Calculate fee based on fee rate | ||
return Math.ceil(vsize * BITCOIN_FEES.DEFAULT_REVEAL_FEE_RATE); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fee calculation is static – transactions with >1 input or a change output will under-pay and be rejected
calculateMemoTransactionFee
assumes exactly one P2WPKH input and one change-less payment output, but bitcoinMakeTransactionWithMemo
may:
- select multiple UTXOs (
pickUtxos.length > 1
), and - append a change output (lines 76-78).
Every extra input adds ~68 vbytes (P2WPKH witness) and every extra P2WPKH output adds 31 vbytes, so the node will relay the tx only if the real fee ≥ required fee. Under-estimation here ⇒ mempool
returns “insufficient fee” and the CLI flow breaks.
-export const calculateMemoTransactionFee = (memoLength: number): number => {
+export const calculateMemoTransactionFee = (
+ memoLength: number,
+ inputCount: number = 1,
+ p2wpkhOutputs: number = 1,
+): number => {
const txOverhead = BITCOIN_TX.TX_OVERHEAD;
- const inputVbytes = 36 + 1 + 43 + 107;
- const outputVbytes = BITCOIN_TX.P2WPKH_OUTPUT_VBYTES;
+ const inputVbytes = inputCount * (36 + 1 + 4 + 107); // base + sequence + witness/4
+ const outputVbytes = p2wpkhOutputs * BITCOIN_TX.P2WPKH_OUTPUT_VBYTES;
const memoOutputVbytes = 9 + memoLength;
const vsize = txOverhead + inputVbytes + outputVbytes + memoOutputVbytes;
return Math.ceil(vsize * BITCOIN_FEES.DEFAULT_REVEAL_FEE_RATE);
};
Call this after UTXO selection, passing pickUtxos.length
and outputs
(1 + (change>0?1:0)
) so the fee is exact.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
export const calculateMemoTransactionFee = (memoLength: number): number => { | |
// Calculate transaction size in vbytes | |
const txOverhead = BITCOIN_TX.TX_OVERHEAD; | |
const inputVbytes = 36 + 1 + 43 + 107; // txin + marker+flag + varint scriptSig len (0) + sequence + witness weight/4 | |
const outputVbytes = BITCOIN_TX.P2WPKH_OUTPUT_VBYTES; | |
const memoOutputVbytes = 9 + memoLength; // varint len + memo length | |
const vsize = txOverhead + inputVbytes + outputVbytes + memoOutputVbytes; | |
// Calculate fee based on fee rate | |
return Math.ceil(vsize * BITCOIN_FEES.DEFAULT_REVEAL_FEE_RATE); | |
}; | |
export const calculateMemoTransactionFee = ( | |
memoLength: number, | |
inputCount: number = 1, | |
p2wpkhOutputs: number = 1, | |
): number => { | |
// Calculate transaction size in vbytes | |
const txOverhead = BITCOIN_TX.TX_OVERHEAD; | |
const inputVbytes = inputCount * (36 + 1 + 4 + 107); // base + sequence + witness/4 | |
const outputVbytes = p2wpkhOutputs * BITCOIN_TX.P2WPKH_OUTPUT_VBYTES; | |
const memoOutputVbytes = 9 + memoLength; // varint len + memo length | |
const vsize = txOverhead + inputVbytes + outputVbytes + memoOutputVbytes; | |
// Calculate fee based on fee rate | |
return Math.ceil(vsize * BITCOIN_FEES.DEFAULT_REVEAL_FEE_RATE); | |
}; |
🤖 Prompt for AI Agents
In utils/bitcoinMemo.helpers.ts around lines 18 to 28, the fee calculation
assumes exactly one input and one output, but the transaction may have multiple
inputs and a change output, causing underpayment. Modify
calculateMemoTransactionFee to accept the number of inputs and outputs as
parameters, then compute the fee by adding the appropriate vbytes for each extra
input (~68 vbytes) and output (~31 vbytes). Call this function after UTXO
selection, passing the actual input count and output count (including change
output if present) to ensure the fee is accurately calculated.
} else if (options.method === "memo") { | ||
const memo = options.data?.startsWith("0x") | ||
? options.data.slice(2) | ||
: options.data; | ||
|
||
await createAndBroadcastTransactions( | ||
key, | ||
utxos, | ||
address, | ||
data, | ||
options.api, | ||
amount, | ||
options.gateway | ||
); | ||
const tx = await bitcoinMakeTransactionWithMemo( | ||
options.gateway, | ||
key, | ||
BITCOIN_LIMITS.DUST_THRESHOLD.P2WPKH, | ||
utxos, | ||
address, | ||
options.api, | ||
memo | ||
); | ||
const txid = await broadcastBtcTransaction(tx, options.api); | ||
console.log(`Transaction hash: ${txid}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
memo
flow ignores --amount
and skips user confirmation
-
The gateway payment is hard-coded to the dust threshold (
294 sat
) instead of the user-specified--amount
. This breaks the economic intent of deposit-and-call. -
Unlike the inscription path, no
displayAndConfirmMemoTransaction
is invoked, so the user cannot preview fees or payload. -
memo
may beundefined
when--data
is omitted, causingBuffer.from(undefined)
to throw downstream.
-const tx = await bitcoinMakeTransactionWithMemo(
- options.gateway,
- key,
- BITCOIN_LIMITS.DUST_THRESHOLD.P2WPKH,
+const amountSat = parseAmount(options.amount); // reuse helper
+await displayAndConfirmMemoTransaction(
+ amountSat,
+ options.gateway,
+ address,
+ memo
+);
+const tx = await bitcoinMakeTransactionWithMemo(
+ options.gateway,
+ key,
+ amountSat,
utxos,
address,
options.api,
memo
);
Add a guard:
if (!options.data) {
throw new Error("`--data` is required when --method memo");
}
🤖 Prompt for AI Agents
In packages/commands/src/bitcoin/depositAndCall.ts around lines 100 to 115, the
memo method incorrectly uses a hardcoded dust threshold amount instead of the
user-specified --amount, skips user confirmation by not calling
displayAndConfirmMemoTransaction, and does not guard against missing --data
causing undefined memo. Fix this by replacing the hardcoded amount with the
user-provided amount, adding a call to displayAndConfirmMemoTransaction to allow
user preview and confirmation, and adding a guard at the start of the memo flow
that throws an error if options.data is missing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/commands/src/bitcoin/depositAndCall.ts (1)
100-116
:⚠️ Potential issue
memo
flow ignores--amount
and skips user confirmation
The gateway payment is hard-coded to the dust threshold (
294 sat
) instead of the user-specified--amount
. This breaks the economic intent of deposit-and-call.Unlike the inscription path, no transaction preview or confirmation is shown to the user.
memo
may beundefined
when--data
is omitted, causing runtime errors.+if (!options.data) { + throw new Error("`--data` is required when --method memo"); +} const memo = options.data?.startsWith("0x") ? options.data.slice(2) : options.data; +const amountSat = parseAmount(options.amount); +await displayAndConfirmMemoTransaction( + amountSat, + options.gateway, + address, + memo +); const tx = await bitcoinMakeTransactionWithMemo( options.gateway, key, - BITCOIN_LIMITS.DUST_THRESHOLD.P2WPKH, + amountSat, utxos, address, options.api, memo );
🧹 Nitpick comments (1)
packages/commands/src/bitcoin/depositAndCall.ts (1)
125-127
: Update command description to reflect the new memo methodThe command description still only mentions inscriptions, but the command now supports both inscription and memo methods.
.description( - "Deposit BTC and call a contract on ZetaChain (using inscriptions)" + "Deposit BTC and call a contract on ZetaChain (using inscriptions or memos)" )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/commands/src/bitcoin/call.ts
(3 hunks)packages/commands/src/bitcoin/deposit.ts
(3 hunks)packages/commands/src/bitcoin/depositAndCall.ts
(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/commands/src/bitcoin/call.ts
- packages/commands/src/bitcoin/deposit.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/commands/src/bitcoin/depositAndCall.ts (5)
utils/bitcoinEncode.ts (2)
bitcoinEncode
(57-71)trimOx
(183-185)types/bitcoin.constants.ts (1)
BITCOIN_LIMITS
(40-48)utils/bitcoin.helpers.ts (1)
calculateFees
(251-275)utils/bitcoin.command.helpers.ts (3)
displayAndConfirmTransaction
(96-119)createAndBroadcastTransactions
(163-203)broadcastBtcTransaction
(149-158)utils/bitcoinMemo.helpers.ts (1)
bitcoinMakeTransactionWithMemo
(30-102)
🔇 Additional comments (4)
packages/commands/src/bitcoin/depositAndCall.ts (4)
71-71
: Confirms deposit fee is now properly calculated for inscription transactionsThe code correctly calculates the deposit fee for inscription transactions using the
calculateFees
function, which is important for ensuring transactions are processed correctly by the network.
97-98
: Deposit fee correctly added to transaction amountThe code properly includes the deposit fee in the total amount sent in the transaction, which addresses the issue mentioned in the PR comments where transactions were failing due to insufficient fees.
39-66
: Improved error handling for inscription methodThe code now properly validates that either the necessary ABI parameters or raw data are provided for inscription transactions, with a clear error message when neither is available.
74-89
: Transaction details now include deposit fee in confirmation displayThe confirmation display for inscription transactions now includes the deposit fee, which provides better transparency to users about all costs involved in the transaction.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
♻️ Duplicate comments (1)
packages/commands/src/bitcoin/depositAndCall.ts (1)
145-153
: 🛠️ Refactor suggestionMissing required
--data
guard and possibleundefined
memo crash
memo
may beundefined
if the user forgets--data
, which later causes
Buffer.from(undefined, "hex")
to throw inside
bitcoinMakeTransactionWithMemo
.
Add an explicit check before trimming:-if (options.method === "memo") { - const memo = options.data?.startsWith("0x") - ? options.data.slice(2) - : options.data; +if (options.method === "memo") { + if (!options.data) { + throw new Error("`--data` is required when --method memo"); + } + const memo = options.data.startsWith("0x") + ? options.data.slice(2) + : options.data;
🧹 Nitpick comments (5)
utils/bitcoin.command.helpers.ts (1)
125-150
:displayAndConfirmMemoTransaction
hard-codes “Signet” and silently accepts huge numbers
- Network is fixed to “Signet”, but the helper is reusable for Testnet / Mainnet.
- The function receives
amount
/fee
as JS numbers which can silently lose
precision above 53-bit (> 0.09 BTC
). All other helpers use theparseAmount
guard to ensure safety.Consider:
-export const displayAndConfirmMemoTransaction = async ( - amount: number, +export const displayAndConfirmMemoTransaction = async ( + amountSat: number, fee: number, gateway: string, sender: string, memo: string, - network = "Signet" +) => +{ + if (!Number.isSafeInteger(amountSat) || !Number.isSafeInteger(fee)) { + throw new Error("Amount or fee exceeds JS safe-integer range"); + }And interpolate
network
instead of the constant.packages/commands/src/bitcoin/depositAndCall.ts (1)
162-171
: Memo flow bypasses user confirmation on fee changes
bitcoinMakeTransactionWithMemo
might change the selected UTXOs and therefore
the actual fee paid, but the confirmation screen is shown before the PSBT
is built. Either:
- move
displayAndConfirmMemoTransaction
after PSBT creation, or- calculate the exact fee (
sum(inputs) - amount - change
) and re-confirm.utils/bitcoin.helpers.ts (3)
134-158
: Duplicate deposit-fee formula – centralise to avoid drift
makeCommitTransaction
in-lines
Math.ceil((68 * 2 * revealFee) / vsize)
while a dedicated
calculateDepositFee()
helper exists lower in the same file. Replace the
inline maths with the helper to keep a single source of truth.-const depositFee = Math.ceil((68 * 2 * revealFee) / vsize); +const depositFee = calculateDepositFee(revealFee, vsize);
195-220
:calculateRevealFee
duplicates constant maths across codebaseVery similar code exists in multiple helpers. Consolidate into one utility
module to avoid subtle divergence when fee policy changes.
285-301
:calculateFees
mixes static commit fee with dynamic feesThe function queries
getDepositFee
(network round-trip) but still hard-codes
commitFee
. Either fetch a dynamic commit-fee-rate too, or make it clear this
is intentional (e.g. parameterise the constant).
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/commands/src/bitcoin/call.ts
(2 hunks)packages/commands/src/bitcoin/deposit.ts
(2 hunks)packages/commands/src/bitcoin/depositAndCall.ts
(2 hunks)utils/bitcoin.command.helpers.ts
(5 hunks)utils/bitcoin.helpers.ts
(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/commands/src/bitcoin/call.ts
- packages/commands/src/bitcoin/deposit.ts
🧰 Additional context used
🧬 Code Graph Analysis (3)
utils/bitcoin.command.helpers.ts (1)
types/bitcoin.types.ts (1)
bitcoinMethods
(55-55)
packages/commands/src/bitcoin/depositAndCall.ts (5)
utils/bitcoin.command.helpers.ts (4)
fetchUtxos
(84-89)displayAndConfirmTransaction
(94-123)broadcastBtcTransaction
(155-164)displayAndConfirmMemoTransaction
(128-150)utils/bitcoinEncode.ts (2)
bitcoinEncode
(57-71)trimOx
(183-185)types/bitcoin.constants.ts (2)
BITCOIN_FEES
(54-58)BITCOIN_LIMITS
(40-49)utils/bitcoin.helpers.ts (3)
makeCommitTransaction
(97-185)calculateRevealFee
(195-220)makeRevealTransaction
(235-278)utils/bitcoinMemo.helpers.ts (2)
getDepositFee
(19-30)bitcoinMakeTransactionWithMemo
(32-104)
utils/bitcoin.helpers.ts (3)
types/bitcoin.constants.ts (3)
BITCOIN_FEES
(54-58)BITCOIN_TX
(28-35)BITCOIN_LIMITS
(40-49)types/bitcoin.types.ts (1)
BtcUtxo
(11-21)utils/bitcoinMemo.helpers.ts (1)
getDepositFee
(19-30)
🔇 Additional comments (2)
utils/bitcoin.command.helpers.ts (1)
216-240
: Option set now mixes kebab-case & camel-case – keep it consistent
--bitcoin-api
&--gas-price-api
are kebab-case but the rest of the toolkit
uses camel-case (--api
,--privateKey
, …). This is only a stylistic
inconsistency but will surprise users and break existing scripts.Either:
- revert to
--api
+--gasPriceApi
, or- adopt kebab-case everywhere and provide deprecated aliases.
packages/commands/src/bitcoin/depositAndCall.ts (1)
150-153
: Fee logic setsfeeTotal
to dust instead of amount + fee below threshold
feeTotal = dust
discards the already-fetchedfee
, making the total
amount + dust
, notdust
in total. Iffee
is close todust
, the user
overpays.-const feeTotal = amount + fee <= dust ? dust : fee; +const feeTotal = amount + fee <= dust ? dust - amount : fee;(or keep the old variables but clarify intent).
info.amount | ||
? `Amount: ${info.amount} BTC (${ethers.parseUnits(info.amount, 8)} sats)` | ||
: "" | ||
} | ||
Inscription Commit Fee: ${info.inscriptionCommitFee} sats | ||
Inscription Reveal Fee: ${info.inscriptionRevealFee} sats | ||
Deposit Fee: ${info.depositFee} sats | ||
Total: ${ | ||
Number(ethers.parseUnits(info.amount, 8)) + | ||
info.inscriptionCommitFee + | ||
info.inscriptionRevealFee + | ||
info.depositFee | ||
} sats |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Total
calculation crashes when amount
is omitted
ethers.parseUnits(info.amount, 8)
is evaluated unconditionally inside the
template literal even when info.amount
is falsy.
undefined
(or ""
) will cause parseUnits
to throw and the whole CLI to
terminate for flows that legitimately have no explicit amount
(e.g. call
-only commands).
-Total: ${
- Number(ethers.parseUnits(info.amount, 8)) +
+const amountSats =
+ info.amount !== undefined ? Number(ethers.parseUnits(info.amount, 8)) : 0;
+Total: ${
+ amountSats +
info.inscriptionCommitFee +
info.inscriptionRevealFee +
info.depositFee
} sats
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In utils/bitcoin.command.helpers.ts around lines 100 to 112, the Total
calculation uses ethers.parseUnits(info.amount, 8) without checking if
info.amount is defined, causing a crash when amount is omitted. Fix this by
conditionally parsing the amount only if info.amount is truthy; otherwise, treat
the amount as zero in the total sum calculation to prevent parseUnits from
throwing an error.
const amount = Number(ethers.parseUnits(options.amount, 8)); | ||
const inscriptionFee = BITCOIN_FEES.DEFAULT_COMMIT_FEE_SAT; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unsafe BigInt → Number cast can overflow for large deposits
Number(ethers.parseUnits(options.amount, 8))
silently truncates for values
≥ 2 ¹⁵³
sats (≈ 90 BTC). Use the existing parseAmount()
helper which guards
against overflow.
-const amount = Number(ethers.parseUnits(options.amount, 8));
+const amount = parseAmount(options.amount);
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
const amount = Number(ethers.parseUnits(options.amount, 8)); | |
const inscriptionFee = BITCOIN_FEES.DEFAULT_COMMIT_FEE_SAT; | |
const amount = parseAmount(options.amount); | |
const inscriptionFee = BITCOIN_FEES.DEFAULT_COMMIT_FEE_SAT; |
🤖 Prompt for AI Agents
In packages/commands/src/bitcoin/depositAndCall.ts around lines 78 to 80,
replace the unsafe cast from BigInt to Number using
Number(ethers.parseUnits(options.amount, 8)) with the safer parseAmount() helper
function to prevent overflow issues for large deposit values. This ensures the
amount is parsed correctly without truncation for values exceeding 2^53 sats.
export const getTxVirtualSize = (tx: bitcoin.Transaction): number => { | ||
const baseSize = tx.byteLength(false); // no witness data | ||
const totalSize = tx.byteLength(); // with witness data | ||
const weight = baseSize * 3 + totalSize; | ||
const vSize = Math.ceil(weight / 4); | ||
return vSize; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getTxVirtualSize
uses a non-existent byteLength(false)
overload
bitcoin.Transaction.byteLength()
accepts an options object
({ disableWitness: boolean }
) in recent bitcoinjs-lib
versions. Passing a
boolean will compile but returns NaN
at runtime on older versions.
A simpler, version-agnostic approach:
-export const getTxVirtualSize = (tx: bitcoin.Transaction): number => {
- const baseSize = tx.byteLength(false);
- const totalSize = tx.byteLength();
- const weight = baseSize * 3 + totalSize;
- const vSize = Math.ceil(weight / 4);
- return vSize;
-};
+export const getTxVirtualSize = (tx: bitcoin.Transaction): number => {
+ return tx.virtualSize(); // bitcoinjs-lib v6+
+};
If you need to stay compatible with v5, pass { disableWitness: true }
instead.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
export const getTxVirtualSize = (tx: bitcoin.Transaction): number => { | |
const baseSize = tx.byteLength(false); // no witness data | |
const totalSize = tx.byteLength(); // with witness data | |
const weight = baseSize * 3 + totalSize; | |
const vSize = Math.ceil(weight / 4); | |
return vSize; | |
}; | |
export const getTxVirtualSize = (tx: bitcoin.Transaction): number => { | |
return tx.virtualSize(); // bitcoinjs-lib v6+ | |
}; |
🤖 Prompt for AI Agents
In utils/bitcoin.helpers.ts around lines 187 to 193, the getTxVirtualSize
function incorrectly calls tx.byteLength(false), but byteLength expects an
options object like { disableWitness: true } for compatibility. Update the call
to tx.byteLength({ disableWitness: true }) to ensure it works correctly across
bitcoinjs-lib versions and avoid runtime NaN errors.
I'm going to be splitting the functionality into two namespaces:
There are just too many inscription/memo-specific params to keep this functionality in the same command. We can use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
♻️ Duplicate comments (4)
utils/bitcoinMemo.helpers.ts (3)
43-43
: Address network constant inconsistency.The code uses
TESTNET
while other Bitcoin helpers in the codebase useSIGNET
. This inconsistency could lead to network rule mismatches.This issue was flagged in previous reviews but appears unresolved. Please use the
SIGNET
constant for consistency with other helpers.
44-44
: Add hex string validation for memo.The
Buffer.from(m, "hex")
call can throw opaque errors for invalid hex strings. This was flagged in previous reviews.Add validation before the Buffer.from call to provide clear error messages for invalid hex input.
76-78
: Handle dust threshold for change output.A change amount below the dust threshold makes the transaction non-standard. This was previously identified but remains unaddressed.
Only add the change output if the change is greater than or equal to
BITCOIN_LIMITS.DUST_THRESHOLD.P2WPKH
, otherwise add the change to the fee.utils/bitcoin.command.helpers.ts (1)
107-111
:⚠️ Potential issueTotal calculation still crashes when amount is falsy.
The existing issue remains unresolved. Even though
amount
is marked as required in the interface, the conditional rendering suggests it might be an empty string in some cases, which would causeethers.parseUnits
to throw.Apply the previously suggested fix:
+const amountSats = info.amount ? Number(ethers.parseUnits(info.amount, 8)) : 0; Total: ${ - Number(ethers.parseUnits(info.amount, 8)) + + amountSats + info.inscriptionCommitFee + info.inscriptionRevealFee + info.depositFee } sats
🧹 Nitpick comments (9)
packages/commands/src/bitcoin/memo/encode.ts (1)
30-33
: Remove redundant null checks.The schema already provides default empty arrays for
types
andvalues
, making these checks unnecessary.const main = (options: EncodeOptions) => { - // Ensure types and values are arrays even if not provided - const types = options.types || []; - const values = options.values || []; + const { types, values } = options;packages/commands/src/bitcoin/memo/call.ts (1)
33-41
: Extract common hex prefix removal logic.The "0x" prefix removal logic is duplicated. Consider extracting it to reduce code duplication.
- const data = options.data?.startsWith("0x") - ? options.data.slice(2) - : options.data; - - const receiver = options.receiver?.startsWith("0x") - ? options.receiver.slice(2) - : options.receiver; + const removeHexPrefix = (str?: string) => str?.startsWith("0x") ? str.slice(2) : str; + + const data = removeHexPrefix(options.data); + const receiver = removeHexPrefix(options.receiver);packages/commands/src/bitcoin/inscription/call.ts (1)
107-133
: Well-implemented two-phase transaction flow!The commit and reveal transactions are properly sequenced with correct fee handling. Note that the reveal transaction depends on the commit transaction being confirmed.
Consider adding retry logic or confirmation waiting for the commit transaction before attempting the reveal, as network delays could cause issues.
packages/commands/src/bitcoin/inscription/deposit.ts (2)
75-75
: Clarify the deposit fee calculation formula.The deposit fee calculation uses the formula
Math.ceil((68 * 2 * revealFee) / vsize)
which appears arbitrary and lacks documentation. This magic number approach makes it difficult to understand the fee calculation rationale.Consider adding a comment explaining the formula or extracting it to a named constant with documentation:
+// Deposit fee calculation: 68 bytes * 2 * reveal fee rate / virtual size +// 68 represents the estimated size of a P2WPKH input in vbytes const depositFee = Math.ceil((68 * 2 * revealFee) / vsize);
37-52
: Consider validating receiver address format.While the code handles both receiver/revert address and raw data scenarios, there's no validation that the receiver address is a valid ZetaChain address format when provided.
Consider adding basic address format validation:
let data; if (options.receiver && revertAddress) { + // Basic validation for ZetaChain address format + if (options.receiver && !options.receiver.match(/^0x[a-fA-F0-9]{40}$/)) { + throw new Error("Invalid receiver address format. Expected 40-character hex address with 0x prefix"); + } data = Buffer.from( bitcoinEncode( options.receiver, Buffer.from(""), // Empty payload for deposit revertAddress, OpCode.Deposit, EncodingFormat.EncodingFmtABI ), "hex" );utils/bitcoinMemo.helpers.ts (2)
19-30
: Fee calculation in getDepositFee lacks documentation.The formula
medianGasPrice * 68
uses a magic number (68) without explanation. This makes it difficult to understand the fee calculation basis.Add documentation explaining the calculation:
+ // Calculate deposit fee: median gas price * 68 vbytes (estimated P2WPKH input size) return medianGasPrice * 68;
21-29
: Improve error handling for API failures.The current error handling re-throws the original error, which may not provide sufficient context for debugging API failures.
Enhance error handling with more context:
} catch (error) { - console.error("Error fetching gas price:", error); - throw error; + const message = error instanceof Error ? error.message : 'Unknown error'; + throw new Error(`Failed to fetch deposit fee from ${api}: ${message}`); }packages/commands/src/bitcoin/memo/depositAndCall.ts (1)
72-74
: Fix incorrect comment about Bitcoin inscriptions.The comment mentions "Bitcoin inscriptions" but this command uses memo (OP_RETURN) transactions, not inscriptions.
/** * Command definition for deposit-and-call - * This allows users to deposit BTC and call a contract on ZetaChain using Bitcoin inscriptions + * This allows users to deposit BTC and call a contract on ZetaChain using Bitcoin memo transactions */packages/commands/src/bitcoin/inscription/depositAndCall.ts (1)
92-92
: Add documentation for the deposit fee calculation formula.The formula
Math.ceil((68 * 2 * revealFee) / vsize)
uses magic numbers that should be documented for maintainability.Add a comment explaining the calculation:
+// Calculate deposit fee based on ZetaChain's fee structure +// 68 * 2 represents the estimated vbytes for the deposit transaction on ZetaChain const depositFee = Math.ceil((68 * 2 * revealFee) / vsize);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
packages/commands/src/bitcoin/deposit.ts
(0 hunks)packages/commands/src/bitcoin/index.ts
(1 hunks)packages/commands/src/bitcoin/inscription/call.ts
(4 hunks)packages/commands/src/bitcoin/inscription/deposit.ts
(1 hunks)packages/commands/src/bitcoin/inscription/depositAndCall.ts
(5 hunks)packages/commands/src/bitcoin/inscription/encode.ts
(1 hunks)packages/commands/src/bitcoin/inscription/index.ts
(1 hunks)packages/commands/src/bitcoin/memo/call.ts
(1 hunks)packages/commands/src/bitcoin/memo/deposit.ts
(1 hunks)packages/commands/src/bitcoin/memo/depositAndCall.ts
(1 hunks)packages/commands/src/bitcoin/memo/encode.ts
(1 hunks)packages/commands/src/bitcoin/memo/index.ts
(1 hunks)types/bitcoin.types.ts
(3 hunks)utils/bitcoin.command.helpers.ts
(3 hunks)utils/bitcoinMemo.helpers.ts
(1 hunks)
💤 Files with no reviewable changes (1)
- packages/commands/src/bitcoin/deposit.ts
✅ Files skipped from review due to trivial changes (3)
- packages/commands/src/bitcoin/inscription/encode.ts
- packages/commands/src/bitcoin/memo/index.ts
- packages/commands/src/bitcoin/inscription/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- types/bitcoin.types.ts
🧰 Additional context used
🧬 Code Graph Analysis (5)
packages/commands/src/bitcoin/index.ts (2)
packages/commands/src/bitcoin/inscription/index.ts (1)
inscriptionCommand
(8-15)packages/commands/src/bitcoin/memo/index.ts (1)
memoCommand
(8-15)
packages/commands/src/bitcoin/inscription/call.ts (5)
types/bitcoin.types.ts (1)
inscriptionCallOptionsSchema
(92-112)utils/bitcoin.command.helpers.ts (3)
fetchUtxos
(83-88)displayAndConfirmTransaction
(93-122)broadcastBtcTransaction
(156-165)types/bitcoin.constants.ts (1)
BITCOIN_FEES
(54-58)utils/bitcoin.helpers.ts (3)
makeCommitTransaction
(97-185)calculateRevealFee
(195-220)makeRevealTransaction
(235-278)utils/validateAndParseSchema.ts (1)
validateAndParseSchema
(28-59)
packages/commands/src/bitcoin/inscription/depositAndCall.ts (4)
types/bitcoin.types.ts (1)
inscriptionDepositAndCallOptionsSchema
(55-76)utils/bitcoin.command.helpers.ts (3)
fetchUtxos
(83-88)displayAndConfirmTransaction
(93-122)broadcastBtcTransaction
(156-165)types/bitcoin.constants.ts (1)
BITCOIN_FEES
(54-58)utils/bitcoin.helpers.ts (3)
makeCommitTransaction
(97-185)calculateRevealFee
(195-220)makeRevealTransaction
(235-278)
packages/commands/src/bitcoin/memo/encode.ts (2)
types/shared.schema.ts (1)
typesAndValuesLengthRefineRule
(72-88)utils/bitcoinEncode.ts (2)
trimOx
(183-185)bitcoinEncode
(57-71)
utils/bitcoinMemo.helpers.ts (1)
types/bitcoin.types.ts (2)
BtcUtxo
(11-21)BtcTxById
(38-53)
🔇 Additional comments (19)
packages/commands/src/bitcoin/index.ts (2)
3-5
: LGTM! Clean import structure for the new command groups.The imports correctly reference the new inscription and memo command groups, following a logical organization.
6-10
: Well-structured command organization!The restructuring into inscription and memo command groups provides clear separation of concerns and improves usability.
packages/commands/src/bitcoin/memo/encode.ts (2)
1-29
: Good schema design with proper validation!The schema correctly validates the relationship between types and values arrays using a shared validation rule, promoting code reusability.
57-74
: Well-structured command definition!The command properly configures options with appropriate defaults and correctly filters enum choices to exclude numeric keys.
packages/commands/src/bitcoin/memo/call.ts (1)
43-69
: Solid transaction flow implementation!The implementation correctly handles fee calculation, user confirmation, and transaction broadcasting with appropriate error handling delegated to helper functions.
packages/commands/src/bitcoin/inscription/call.ts (3)
5-27
: Clean import restructuring!The imports are well-organized and properly reference the new inscription-specific schema and helper functions.
69-90
: Proper implementation of commit and fee calculation!The commit transaction creation and reveal fee calculation correctly use the new helper functions with appropriate constants.
140-171
: Consistent command structure with proper validation!The command definition maintains consistency with the memo version while using the appropriate inscription-specific schema.
packages/commands/src/bitcoin/inscription/deposit.ts (1)
125-153
: LGTM! Well-structured command definition.The command definition follows good CLI patterns with proper option conflicts, required options, and validation. The use of Zod schema validation ensures type safety.
packages/commands/src/bitcoin/memo/deposit.ts (1)
66-85
: LGTM! Clean command structure.The command definition is well-structured with appropriate options and validation. The use of
requiredOption
for receiver ensures the critical validation issue above is addressed at the CLI level.packages/commands/src/bitcoin/memo/depositAndCall.ts (1)
76-99
: LGTM! Well-structured command definition.The command definition follows good CLI patterns with appropriate options and validation using Zod schemas.
packages/commands/src/bitcoin/inscription/depositAndCall.ts (5)
5-29
: Import refactoring looks good!The updated imports properly reflect the architectural changes for the two-step commit-reveal transaction flow and the renamed constants/schemas.
42-42
: API option rename is consistent.The change from
options.api
tooptions.bitcoinApi
correctly aligns with the updated schema.
74-82
: Commit transaction creation follows the correct pattern.The transaction is properly created with all required parameters, and the fee calculation is handled internally by
makeCommitTransaction
.
94-136
: Transaction flow correctly implements the two-step commit-reveal pattern.The implementation properly:
- Displays all fee components to the user
- Broadcasts the commit transaction first
- Uses the commit txid for the reveal transaction
- Includes all fees in the reveal transaction value
145-167
: Command definition updates are appropriate.The description is clearer and the schema validation correctly uses the updated
inscriptionDepositAndCallOptionsSchema
.utils/bitcoin.command.helpers.ts (3)
27-40
: Interface updates align with the new fee structure.The updated
TransactionInfo
interface properly reflects the separation of inscription and deposit fees, which provides better transparency to users.
124-152
: New memo transaction display function is well-implemented.The function provides clear fee breakdowns and proper satoshi-to-BTC conversions for user-friendly display.
217-226
: CLI option improvements enhance clarity and functionality.The renamed
--bitcoin-api
option and new--gas-price-api
option properly support the enhanced fee calculation features while maintaining sensible defaults.
const result = bitcoinEncode( | ||
options.receiver, | ||
payloadBuffer, | ||
options.revertAddress, | ||
OpCode[options.opCode as keyof typeof OpCode], | ||
EncodingFormat[options.format as keyof typeof EncodingFormat] | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add validation for enum values to prevent runtime errors.
The current type assertions could fail if invalid enum values are provided. Consider validating the enum values first.
+ const opCode = options.opCode ? OpCode[options.opCode as keyof typeof OpCode] : OpCode.DepositAndCall;
+ const format = options.format ? EncodingFormat[options.format as keyof typeof EncodingFormat] : EncodingFormat.EncodingFmtABI;
+
+ if (options.opCode && opCode === undefined) {
+ throw new Error(`Invalid operation code: ${options.opCode}`);
+ }
+ if (options.format && format === undefined) {
+ throw new Error(`Invalid encoding format: ${options.format}`);
+ }
+
// Encode the data
const result = bitcoinEncode(
options.receiver,
payloadBuffer,
options.revertAddress,
- OpCode[options.opCode as keyof typeof OpCode],
- EncodingFormat[options.format as keyof typeof EncodingFormat]
+ opCode,
+ format
);
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
const result = bitcoinEncode( | |
options.receiver, | |
payloadBuffer, | |
options.revertAddress, | |
OpCode[options.opCode as keyof typeof OpCode], | |
EncodingFormat[options.format as keyof typeof EncodingFormat] | |
); | |
const opCode = options.opCode | |
? OpCode[options.opCode as keyof typeof OpCode] | |
: OpCode.DepositAndCall; | |
const format = options.format | |
? EncodingFormat[options.format as keyof typeof EncodingFormat] | |
: EncodingFormat.EncodingFmtABI; | |
if (options.opCode && opCode === undefined) { | |
throw new Error(`Invalid operation code: ${options.opCode}`); | |
} | |
if (options.format && format === undefined) { | |
throw new Error(`Invalid encoding format: ${options.format}`); | |
} | |
// Encode the data | |
const result = bitcoinEncode( | |
options.receiver, | |
payloadBuffer, | |
options.revertAddress, | |
opCode, | |
format | |
); |
🤖 Prompt for AI Agents
In packages/commands/src/bitcoin/memo/encode.ts around lines 46 to 52, the code
uses type assertions to access enum values without validation, which can cause
runtime errors if invalid values are passed. Add checks to verify that
options.opCode and options.format exist as keys in the OpCode and EncodingFormat
enums before using them. If the values are invalid, handle the error
appropriately, such as throwing an error or returning a failure response, to
prevent runtime exceptions.
export const callCommand = new Command() | ||
.name("call") | ||
.description("Call a contract on ZetaChain") | ||
.option("-r, --receiver <address>", "ZetaChain receiver address") | ||
.requiredOption( | ||
"-g, --gateway <address>", | ||
"Bitcoin gateway (TSS) address", | ||
"tb1qy9pqmk2pd9sv63g27jt8r657wy0d9ueeh0nqur" | ||
) | ||
.addOption(new Option("--data <data>", "Pass raw data")) | ||
.option("--network-fee <fee>", "Network fee (in sats)", "1750") | ||
.action(async (opts) => { | ||
const validated = validateAndParseSchema(opts, memoCallOptionsSchema, { | ||
exitOnError: true, | ||
}); | ||
await main(validated); | ||
}); | ||
|
||
addCommonOptions(callCommand); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Verification agent
🧩 Analysis chain
Verify the default gateway address is current.
The command definition looks good with appropriate defaults. However, please ensure the hardcoded gateway address is the correct current testnet address.
🌐 Web query:
What is the current Bitcoin testnet gateway address for ZetaChain?
💡 Result:
The current Bitcoin testnet gateway address for ZetaChain is:
tb1q8ev0a9c0khvumur5w6dw9szuzk9a6f7lh6jlhz
This address is used for depositing Bitcoin testnet (tBTC) into ZetaChain's Universal EVM. To perform a deposit, create a Bitcoin transaction with at least two outputs:
-
Send the desired amount of tBTC to the TSS Bitcoin address (
tb1q8ev0a9c0khvumur5w6dw9szuzk9a6f7lh6jlhz
). -
Include a memo output (
OP_RETURN
) containing the recipient address on ZetaChain's EVM.
For detailed instructions on depositing tBTC into ZetaChain's Universal EVM, refer to the ZetaChain documentation: (zetachain.com)
Citations:
Update default Bitcoin testnet gateway address
The hardcoded testnet gateway address appears to be outdated. According to the latest ZetaChain docs [1], the current Bitcoin testnet gateway (TSS) address is tb1q8ev0a9c0khvumur5w6dw9szuzk9a6f7lh6jlhz
. Please update:
• File: packages/commands/src/bitcoin/memo/call.ts
Line ~82: change default gateway address
Suggested diff:
.requiredOption(
- "-g, --gateway <address>",
- "Bitcoin gateway (TSS) address",
- "tb1qy9pqmk2pd9sv63g27jt8r657wy0d9ueeh0nqur"
+ "-g, --gateway <address>",
+ "Bitcoin gateway (TSS) address",
+ "tb1q8ev0a9c0khvumur5w6dw9szuzk9a6f7lh6jlhz"
)
[1] https://www.zetachain.com/docs/developers/chains/bitcoin/?utm_source=openai
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
export const callCommand = new Command() | |
.name("call") | |
.description("Call a contract on ZetaChain") | |
.option("-r, --receiver <address>", "ZetaChain receiver address") | |
.requiredOption( | |
"-g, --gateway <address>", | |
"Bitcoin gateway (TSS) address", | |
"tb1qy9pqmk2pd9sv63g27jt8r657wy0d9ueeh0nqur" | |
) | |
.addOption(new Option("--data <data>", "Pass raw data")) | |
.option("--network-fee <fee>", "Network fee (in sats)", "1750") | |
.action(async (opts) => { | |
const validated = validateAndParseSchema(opts, memoCallOptionsSchema, { | |
exitOnError: true, | |
}); | |
await main(validated); | |
}); | |
addCommonOptions(callCommand); | |
export const callCommand = new Command() | |
.name("call") | |
.description("Call a contract on ZetaChain") | |
.option("-r, --receiver <address>", "ZetaChain receiver address") | |
.requiredOption( | |
"-g, --gateway <address>", | |
"Bitcoin gateway (TSS) address", | |
"tb1q8ev0a9c0khvumur5w6dw9szuzk9a6f7lh6jlhz" | |
) | |
.addOption(new Option("--data <data>", "Pass raw data")) | |
.option("--network-fee <fee>", "Network fee (in sats)", "1750") | |
.action(async (opts) => { | |
const validated = validateAndParseSchema(opts, memoCallOptionsSchema, { | |
exitOnError: true, | |
}); | |
await main(validated); | |
}); | |
addCommonOptions(callCommand); |
🤖 Prompt for AI Agents
In packages/commands/src/bitcoin/memo/call.ts around lines 75 to 93, update the
default Bitcoin testnet gateway address in the requiredOption for the gateway
from the outdated "tb1qy9pqmk2pd9sv63g27jt8r657wy0d9ueeh0nqur" to the current
address "tb1q8ev0a9c0khvumur5w6dw9szuzk9a6f7lh6jlhz" as per the latest ZetaChain
documentation.
const receiver = options.receiver?.startsWith("0x") | ||
? options.receiver.slice(2) | ||
: options.receiver; | ||
|
||
const memo = receiver || ""; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix potential memo validation issue.
The receiver processing and memo construction has a logical issue. If options.receiver
is undefined, the memo becomes an empty string, but the bitcoinMakeTransactionWithMemo
helper requires at least 20 bytes for the receiver address.
Apply this diff to ensure proper memo construction:
- const receiver = options.receiver?.startsWith("0x")
- ? options.receiver.slice(2)
- : options.receiver;
-
- const memo = receiver || "";
+ if (!options.receiver) {
+ throw new Error("Receiver address is required for memo transactions");
+ }
+
+ const receiver = options.receiver.startsWith("0x")
+ ? options.receiver.slice(2)
+ : options.receiver;
+
+ // Validate receiver is 40 hex characters (20 bytes)
+ if (!/^[a-fA-F0-9]{40}$/.test(receiver)) {
+ throw new Error("Receiver must be a valid 40-character hex address");
+ }
+
+ const memo = receiver;
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
const receiver = options.receiver?.startsWith("0x") | |
? options.receiver.slice(2) | |
: options.receiver; | |
const memo = receiver || ""; | |
if (!options.receiver) { | |
throw new Error("Receiver address is required for memo transactions"); | |
} | |
const receiver = options.receiver.startsWith("0x") | |
? options.receiver.slice(2) | |
: options.receiver; | |
// Validate receiver is 40 hex characters (20 bytes) | |
if (!/^[a-fA-F0-9]{40}$/.test(receiver)) { | |
throw new Error("Receiver must be a valid 40-character hex address"); | |
} | |
const memo = receiver; |
🤖 Prompt for AI Agents
In packages/commands/src/bitcoin/memo/deposit.ts around lines 28 to 32, the memo
is set to an empty string if options.receiver is undefined, which causes
validation issues since bitcoinMakeTransactionWithMemo requires at least 20
bytes for the receiver address. Fix this by ensuring that if options.receiver is
undefined or too short, the memo is constructed with a default or padded value
that meets the 20-byte minimum length requirement before passing it to
bitcoinMakeTransactionWithMemo.
const data = options.data?.startsWith("0x") | ||
? options.data.slice(2) | ||
: options.data; | ||
|
||
const receiver = options.receiver?.startsWith("0x") | ||
? options.receiver.slice(2) | ||
: options.receiver; | ||
|
||
const memo = receiver + (data || ""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Add memo length validation.
The concatenation of receiver and data could result in a memo exceeding the 80-byte limit enforced by bitcoinMakeTransactionWithMemo
. This could cause runtime errors during transaction creation.
Add validation before memo construction:
const receiver = options.receiver?.startsWith("0x")
? options.receiver.slice(2)
: options.receiver;
+ // Validate receiver address format (must be 40 hex chars = 20 bytes)
+ if (!receiver || !/^[a-fA-F0-9]{40}$/.test(receiver)) {
+ throw new Error("Receiver must be a valid 40-character hex address");
+ }
+
const memo = receiver + (data || "");
+
+ // Validate total memo length doesn't exceed 80 bytes
+ if (Buffer.from(memo, "hex").length > 80) {
+ throw new Error("Combined receiver and data exceed 80-byte memo limit. Consider using inscription instead.");
+ }
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
const data = options.data?.startsWith("0x") | |
? options.data.slice(2) | |
: options.data; | |
const receiver = options.receiver?.startsWith("0x") | |
? options.receiver.slice(2) | |
: options.receiver; | |
const memo = receiver + (data || ""); | |
const data = options.data?.startsWith("0x") | |
? options.data.slice(2) | |
: options.data; | |
const receiver = options.receiver?.startsWith("0x") | |
? options.receiver.slice(2) | |
: options.receiver; | |
// Validate receiver address format (must be 40 hex chars = 20 bytes) | |
if (!receiver || !/^[a-fA-F0-9]{40}$/.test(receiver)) { | |
throw new Error("Receiver must be a valid 40-character hex address"); | |
} | |
const memo = receiver + (data || ""); | |
// Validate total memo length doesn't exceed 80 bytes | |
if (Buffer.from(memo, "hex").length > 80) { | |
throw new Error( | |
"Combined receiver and data exceed 80-byte memo limit. Consider using inscription instead." | |
); | |
} |
🤖 Prompt for AI Agents
In packages/commands/src/bitcoin/memo/depositAndCall.ts around lines 34 to 42,
add a validation step before constructing the memo string by concatenating
receiver and data. Calculate the byte length of the combined memo and check if
it exceeds the 80-byte limit enforced by bitcoinMakeTransactionWithMemo. If it
does, throw an error or handle it appropriately to prevent runtime errors during
transaction creation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fadeev This one also seems important.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Memo validation: 2377e0c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@coderabbitai review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
I'll check the current state of the file to review the memo validation implementation you added in commit 2377e0c.
🏁 Script executed:
#!/bin/bash
# Check the current state of the memo depositAndCall file
cat packages/commands/src/bitcoin/memo/depositAndCall.ts
Length of output: 2995
Let me check what was actually implemented in commit 2377e0c to see the memo validation changes:
🏁 Script executed:
#!/bin/bash
# Check the commit details for 2377e0c to see what changes were made
git show 2377e0c --name-only
echo "=== Files changed ==="
git show 2377e0c --stat
echo "=== Detailed changes ==="
git show 2377e0c
Length of output: 751
Let me check the recent commit history to find any memo validation related changes:
🏁 Script executed:
#!/bin/bash
# Check recent commits to find memo validation changes
echo "=== Recent commits ==="
git log --oneline -10
echo -e "\n=== Search for commits mentioning memo validation ==="
git log --oneline --grep="memo" -5
echo -e "\n=== Search for commits mentioning validation ==="
git log --oneline --grep="validation" -5
echo -e "\n=== Check if there are any uncommitted changes to the depositAndCall file ==="
git status packages/commands/src/bitcoin/memo/depositAndCall.ts
Length of output: 655
@fadeev I reviewed the current memo validation implementation. Here's what I found:
Current Validation Status:
✅ Memo validation exists - but it's implemented in the bitcoinMakeTransactionWithMemo
helper function rather than at the input processing level in depositAndCall.ts
.
Current Implementation:
// In bitcoinMakeTransactionWithMemo function:
if (memo.length < 20) throw new Error(errorNoReceiver);
if (memo.length > 80) throw new Error(errorTooLong);
Assessment:
The current approach prevents the runtime error I was concerned about, but has some UX implications:
Pros:
- ✅ Prevents invalid transactions from being created
- ✅ Validation logic is centralized in the helper function
- ✅ Consistent validation across all memo transaction types
Potential UX Improvements:
- Users don't get early feedback about invalid memo lengths
- Error occurs during transaction creation (after fee calculation and confirmation) rather than during input validation
- Could be confusing if users spend time setting up a transaction only to find out the memo is invalid
Recommendation:
The current validation is functionally sufficient to prevent the issue. If you want to improve user experience, consider adding early validation in the command files as originally suggested, but the current implementation adequately addresses the core concern.
The validation is working as intended! 🎉
🧬 Code Graph Analysis Results
Relevant snippets for packages/commands/src/bitcoin/memo/depositAndCall.ts
From types/bitcoin.types.ts
(lines 114-124)
export const memoDepositAndCallOptionsSchema = z.object({
amount: validAmountSchema,
bitcoinApi: z.string().url(),
data: hexStringSchema.optional(),
gasPriceApi: z.string().url(),
gateway: z.string(),
name: z.string().optional().default(DEFAULT_ACCOUNT_NAME),
networkFee: z.string(),
privateKey: z.string().optional(),
receiver: z.string().optional(),
});
- Defines the Zod schema for validating the command options used in
depositAndCall.ts
. - Includes fields like
amount
,bitcoinApi
,data
,gasPriceApi
,gateway
,name
,networkFee
,privateKey
, andreceiver
. - Used for input validation before executing the main logic.
From utils/validateAndParseSchema.ts
(lines 28-59)
export const validateAndParseSchema = <T, U = T>(
args: unknown,
schema: z.ZodType<T, z.ZodTypeDef, U>,
options?: {
exitOnError?: boolean;
shouldLogError?: boolean;
}
): T => {
// Merge with defaults properly - this ensures partial options don't lose default values
const mergedOptions = {
exitOnError: false,
shouldLogError: true,
...options,
};
const result = schema.safeParse(args);
if (!result.success) {
const errorMessage = formatZodError(result.error);
if (mergedOptions.shouldLogError) {
console.error(`\x1b[31m${errorMessage}\x1b[0m`);
}
if (mergedOptions.exitOnError) {
process.exit(1);
} else {
throw new Error(errorMessage);
}
}
return result.data;
};
- Utility function used to validate and parse command options against a Zod schema.
- Logs errors and optionally exits the process on validation failure.
- Used in
depositAndCall.ts
to validate CLI options before running the main function.
From utils/bitcoinMemo.helpers.ts
(lines 19-30 and 32-95)
export const getDepositFee = async (api: string) => {
try {
const response = await axios.get<GasPriceResponse>(`${api}`);
const gasPrice = response.data.GasPrice;
const medianIndex = parseInt(gasPrice.median_index);
const medianGasPrice = parseInt(gasPrice.prices[medianIndex]);
return medianGasPrice * 68;
} catch (error) {
console.error("Error fetching gas price:", error);
throw error;
}
};
- Fetches the deposit fee by querying a gas price API.
- Calculates the fee based on median gas price multiplied by a constant factor (68).
export const bitcoinMakeTransactionWithMemo = async (
gateway: string,
key: bitcoin.Signer,
amount: number,
depositFee: number,
networkFee: number,
utxos: BtcUtxo[],
changeAddr: string,
api: string,
m = ""
) => {
const TESTNET = bitcoin.networks.testnet;
const memo = Buffer.from(m, "hex");
if (memo.length < 20) throw new Error(errorNoReceiver);
if (memo.length > 80) throw new Error(errorTooLong);
utxos.sort((a, b) => a.value - b.value);
const need = amount + depositFee + networkFee;
let sum = 0;
const picked: BtcUtxo[] = [];
for (const u of utxos) {
sum += u.value;
picked.push(u);
if (sum >= need) break;
}
if (sum < need) throw new Error("Not enough funds");
const change = sum - amount - depositFee - networkFee;
const prevTxs = await Promise.all(
picked.map((u) =>
axios.get<BtcTxById>(`${api}/tx/${u.txid}`).then((r) => r.data)
)
);
const psbt = new bitcoin.Psbt({ network: TESTNET });
psbt.addOutput({ address: gateway, value: amount + depositFee });
const embed = bitcoin.payments.embed({ data: [memo] });
if (!embed.output) throw new Error("Unable to embed memo");
psbt.addOutput({ script: embed.output, value: 0 });
if (change > 0) {
psbt.addOutput({ address: changeAddr, value: change });
}
picked.forEach((u, i) => {
psbt.addInput({
hash: prevTxs[i].txid,
index: u.vout,
witnessUtxo: {
script: Buffer.from(prevTxs[i].vout[u.vout].scriptpubkey, "hex"),
value: u.value,
},
});
});
picked.forEach((_, i) => psbt.signInput(i, key));
psbt.finalizeAllInputs();
return psbt.extractTransaction().toHex();
};
- Constructs a Bitcoin transaction with an embedded memo (OP_RETURN).
- Validates memo length (between 20 and 80 bytes).
- Selects UTXOs to cover amount + fees.
- Adds outputs for gateway, memo, and change.
- Signs inputs and returns the raw transaction hex.
From utils/bitcoin.command.helpers.ts
(multiple relevant parts)
export const setupBitcoinKeyPair = (
privateKey: string | undefined,
name: string
): BitcoinKeyPair => {
const keyPrivateKey =
privateKey ||
getAccountData<BitcoinAccountData>("bitcoin", name)?.privateKey;
if (!keyPrivateKey) {
const errorMessage = handleError({
context: "Failed to retrieve private key",
error: new Error("Private key not found"),
shouldThrow: false,
});
throw new Error(errorMessage);
}
// Initialize Bitcoin library with ECC implementation
bitcoin.initEccLib(ecc);
// Set up Bitcoin key pair
const ECPair = ECPairFactory(ecc);
const key = ECPair.fromPrivateKey(Buffer.from(keyPrivateKey, "hex"), {
network: SIGNET,
});
const { address } = bitcoin.payments.p2wpkh({
network: SIGNET,
pubkey: key.publicKey,
});
return { address: address!, key };
};
- Sets up a Bitcoin key pair from a private key or account name.
- Uses Signet network.
- Returns the key pair and associated address.
export const fetchUtxos = async (
address: string,
api: string
): Promise<BtcUtxo[]> => {
return (await axios.get<BtcUtxo[]>(`${api}/address/${address}/utxo`)).data;
};
- Fetches UTXOs for a given Bitcoin address from a Bitcoin API.
export const displayAndConfirmMemoTransaction = async (
amount: number,
networkFee: number,
depositFee: number,
gateway: string,
sender: string,
memo: string
) => {
console.log(`
Network: Signet
Gateway: ${gateway}
Sender: ${sender}
Operation: Memo Transaction
Memo: ${memo}
Deposit Amount: ${amount} sats (${(amount / 100000000).toFixed(8)} BTC)
Network Fee: ${networkFee} sats (${(networkFee / 100000000).toFixed(8)} BTC)
Deposit Fee: ${depositFee} sats (${(depositFee / 100000000).toFixed(8)} BTC)
Deposit Total: ${amount + depositFee} sats (${(
amount / 100000000 +
depositFee / 100000000
).toFixed(8)} BTC)
`);
await confirm({ message: "Proceed?" }, { clearPromptOnDone: true });
};
- Displays transaction details and prompts user for confirmation before proceeding.
export const broadcastBtcTransaction = async (
txHex: string,
api: string
): Promise<string> => {
const { data } = await axios.post<string>(`${api}/tx`, txHex, {
headers: { "Content-Type": "text/plain" },
});
return data;
};
- Broadcasts a raw Bitcoin transaction hex to the Bitcoin network via the API.
- Returns the transaction ID.
export const addCommonOptions = (command: Command) => {
return command
.option(
"--bitcoin-api <url>",
"Bitcoin API",
"https://mempool.space/signet/api"
)
.option(
"--gas-price-api <url>",
"ZetaChain API",
"https://zetachain-athens.blockpi.network/lcd/v1/public/zeta-chain/crosschain/gasPrice/18333"
)
.addOption(
new Option("--private-key <key>", "Bitcoin private key").conflicts([
"name",
])
)
.addOption(
new Option("--name <name>", "Account name")
.default(DEFAULT_ACCOUNT_NAME)
.conflicts(["private-key"])
);
};
- Adds common CLI options to commands, including Bitcoin API URL, gas price API URL, private key, and account name options.
From packages/commands/src/bitcoin/inscription/depositAndCall.ts
(lines 143-173)
export const depositAndCallCommand = new Command()
.name("deposit-and-call")
.description("Deposit BTC and call a contract on ZetaChain")
.option("-r, --receiver <address>", "ZetaChain receiver address")
.requiredOption(
"-g, --gateway <address>",
"Bitcoin gateway (TSS) address",
"tb1qy9pqmk2pd9sv63g27jt8r657wy0d9ueeh0nqur"
)
.option("-t, --types <types...>", "ABI types")
.option("-v, --values <values...>", "Values corresponding to types")
.option("-a, --revert-address <address>", "Revert address")
.requiredOption("--amount <btcAmount>", "BTC amount to send (in BTC)")
.addOption(
new Option("--data <data>", "Pass raw data").conflicts([
"types",
"values",
"revert-address",
"receiver",
])
)
.action(async (opts) => {
const validated = validateAndParseSchema(
opts,
inscriptionDepositAndCallOptionsSchema,
{
exitOnError: true,
}
);
await main(validated);
});
- Shows a similar command definition for a deposit-and-call command but for Bitcoin inscriptions.
- Includes options for ABI types, values, revert address, and conflicts between data and other options.
- Useful for comparison with the memo deposit-and-call command.
Summary
The above snippets provide the schema validation, Bitcoin transaction construction, key setup, UTXO fetching, fee calculation, transaction confirmation, and broadcasting utilities used by the depositAndCall.ts
command. They also show the command option definitions and validation flow. This context is essential for reviewing the depositAndCall.ts
file's correctness and integration with the rest of the codebase.
@@ -63,37 +68,72 @@ const main = async (options: DepositAndCallOptions) => { | |||
); | |||
} | |||
|
|||
const amount = | |||
BITCOIN_LIMITS.MIN_COMMIT_AMOUNT + BITCOIN_LIMITS.ESTIMATED_REVEAL_FEE; | |||
const amount = Number(ethers.parseUnits(options.amount, 8)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential precision loss for large amounts.
Converting the parsed amount to Number
could cause precision loss for amounts exceeding JavaScript's safe integer range (2^53 - 1 satoshis ≈ 90,071 BTC).
Consider adding a safety check:
const amount = Number(ethers.parseUnits(options.amount, 8));
+if (ethers.parseUnits(options.amount, 8) > BigInt(Number.MAX_SAFE_INTEGER)) {
+ throw new Error("Amount exceeds JavaScript safe integer range");
+}
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
const amount = Number(ethers.parseUnits(options.amount, 8)); | |
const amount = Number(ethers.parseUnits(options.amount, 8)); | |
if (ethers.parseUnits(options.amount, 8) > BigInt(Number.MAX_SAFE_INTEGER)) { | |
throw new Error("Amount exceeds JavaScript safe integer range"); | |
} |
🤖 Prompt for AI Agents
In packages/commands/src/bitcoin/inscription/depositAndCall.ts at line 71,
converting the parsed amount to a JavaScript Number risks precision loss for
large values beyond the safe integer range. To fix this, avoid casting to Number
and instead keep the amount as a BigNumber or string type that can safely
represent large values. Additionally, implement a safety check to ensure the
amount does not exceed the maximum safe integer limit before any conversion or
arithmetic operations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fadeev This one seems important.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Safe parsing of amount: 5a1fb6a
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@coderabbitai review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the current content of the depositAndCall.ts file to see the latest version
cat packages/commands/src/bitcoin/inscription/depositAndCall.ts | grep -A 5 -B 5 "const amount"
Length of output: 391
🏁 Script executed:
#!/bin/bash
# Look for the safeParseBitcoinAmount function implementation
rg "safeParseBitcoinAmount" -A 10 -B 2
Length of output: 9276
🏁 Script executed:
#!/bin/bash
# Check if the fix was applied by looking at git history for this file
git log --oneline -10 packages/commands/src/bitcoin/inscription/depositAndCall.ts
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check for any usage of Number() with parseUnits in the codebase
rg "Number\(.*parseUnits" -A 2 -B 2
Length of output: 693
@fadeev Perfect! I can see that you've properly addressed the precision loss issue. The fix looks excellent:
✅ Issue Resolved: The code now uses safeParseBitcoinAmount(options.amount)
instead of the unsafe Number(ethers.parseUnits(options.amount, 8))
.
✅ Proper Implementation: The safeParseBitcoinAmount
function in utils/bitcoin.helpers.ts
includes the exact safety check I suggested:
export const safeParseBitcoinAmount = (
amount: string,
decimals: number = 8
): number => {
const parsedAmount = ethers.parseUnits(amount, decimals);
if (parsedAmount > BigInt(Number.MAX_SAFE_INTEGER)) {
throw new Error("Amount exceeds maximum safe integer limit");
}
return Number(parsedAmount);
};
✅ Consistent Usage: The safe parsing function is now used across multiple files:
packages/commands/src/bitcoin/inscription/depositAndCall.ts
packages/commands/src/bitcoin/inscription/deposit.ts
packages/commands/src/bitcoin/memo/depositAndCall.ts
packages/commands/src/bitcoin/memo/deposit.ts
Minor Note: There are still a couple of unsafe usages in other files that could benefit from the same treatment:
utils/bitcoin.command.helpers.ts
indisplayAndConfirmTransaction
packages/tasks/src/bitcoinDepositAndCall.ts
But those are outside the scope of this specific fix. The core issue has been resolved properly! 🎉
🧬 Code Graph Analysis Results
Relevant snippets for packages/commands/src/bitcoin/inscription/depositAndCall.ts
From types/bitcoin.constants.ts
(lines 54-58)
export const BITCOIN_FEES = {
DEFAULT_COMMIT_FEE_SAT: 15000,
DEFAULT_REVEAL_FEE_RATE: 10,
MIN_TOTAL_FEE: 20000, // Minimum total fee for both transactions
};
- Defines default fee constants used in the deposit-and-call process, including commit fee and reveal fee rate.
From utils/bitcoinEncode.ts
(lines 57-71 and 183-185)
export const bitcoinEncode = (
receiver: Address,
payload: Buffer,
revertAddress: BtcAddress,
opCode: OpCode = OpCode.DepositAndCall,
encodingFormat: EncodingFormat = EncodingFormat.EncodingFmtABI
): string => {
// Create memo header
const header = new Header(encodingFormat, opCode);
// Create memo fields
const fields = new FieldsV0(receiver, payload, revertAddress);
return bytesToHex(encodeToBytes(header, fields));
};
export const trimOx = (hexString: string): string => {
return hexString.startsWith("0x") ? hexString.slice(2) : hexString;
};
bitcoinEncode
encodes receiver, payload, and revert address into a hex string with a memo header and fields.trimOx
removes the "0x" prefix from hex strings, used before encoding payload.
From utils/validateAndParseSchema.ts
(lines 28-59)
export const validateAndParseSchema = <T, U = T>(
args: unknown,
schema: z.ZodType<T, z.ZodTypeDef, U>,
options?: {
exitOnError?: boolean;
shouldLogError?: boolean;
}
): T => {
const mergedOptions = {
exitOnError: false,
shouldLogError: true,
...options,
};
const result = schema.safeParse(args);
if (!result.success) {
const errorMessage = formatZodError(result.error);
if (mergedOptions.shouldLogError) {
console.error(`\x1b[31m${errorMessage}\x1b[0m`);
}
if (mergedOptions.exitOnError) {
process.exit(1);
} else {
throw new Error(errorMessage);
}
}
return result.data;
};
- Validates and parses command options against a Zod schema.
- Logs errors and optionally exits or throws on validation failure.
From types/bitcoin.types.ts
(lines 55-76)
export const inscriptionDepositAndCallOptionsSchema = z
.object({
amount: validAmountSchema,
bitcoinApi: z.string().url(),
data: hexStringSchema.optional(),
gasPriceApi: z.string().url(),
gateway: z.string(),
name: z.string().optional().default(DEFAULT_ACCOUNT_NAME),
privateKey: z.string().optional(),
receiver: z.string().optional(),
revertAddress: z.string().optional(),
types: z.array(z.string()).optional(),
values: z.array(z.string()).optional(),
})
.refine(typesAndValuesLengthRefineRule.rule, {
message: typesAndValuesLengthRefineRule.message,
path: typesAndValuesLengthRefineRule.path,
})
.refine(typesAndDataExclusivityRefineRule.rule, {
message: typesAndDataExclusivityRefineRule.message,
path: typesAndDataExclusivityRefineRule.path,
});
- Zod schema defining the expected options for deposit-and-call command.
- Includes validation refinements for consistency between types/values and data exclusivity.
From utils/bitcoin.command.helpers.ts
(lines 45-78, 83-88, 93-122, 156-165, 215-237)
export const setupBitcoinKeyPair = (
privateKey: string | undefined,
name: string
): BitcoinKeyPair => {
const keyPrivateKey =
privateKey ||
getAccountData<BitcoinAccountData>("bitcoin", name)?.privateKey;
if (!keyPrivateKey) {
const errorMessage = handleError({
context: "Failed to retrieve private key",
error: new Error("Private key not found"),
shouldThrow: false,
});
throw new Error(errorMessage);
}
bitcoin.initEccLib(ecc);
const ECPair = ECPairFactory(ecc);
const key = ECPair.fromPrivateKey(Buffer.from(keyPrivateKey, "hex"), {
network: SIGNET,
});
const { address } = bitcoin.payments.p2wpkh({
network: SIGNET,
pubkey: key.publicKey,
});
return { address: address!, key };
};
export const fetchUtxos = async (
address: string,
api: string
): Promise<BtcUtxo[]> => {
return (await axios.get<BtcUtxo[]>(`${api}/address/${address}/utxo`)).data;
};
export const displayAndConfirmTransaction = async (info: TransactionInfo) => {
const notApplicable = "encoded in raw inscription data";
console.log(`
Network: ${info.network}
${
info.amount
? `Amount: ${info.amount} BTC (${ethers.parseUnits(info.amount, 8)} sats)`
: ""
}
Inscription Commit Fee: ${info.inscriptionCommitFee} sats
Inscription Reveal Fee: ${info.inscriptionRevealFee} sats
Deposit Fee: ${info.depositFee} sats
Total: ${
Number(ethers.parseUnits(info.amount, 8)) +
info.inscriptionCommitFee +
info.inscriptionRevealFee +
info.depositFee
} sats
Gateway: ${info.gateway}
Sender: ${info.sender}
Receiver: ${info.receiver || notApplicable}
Revert Address: ${info.revertAddress || notApplicable}
Operation: ${info.operation}
${info.encodedMessage ? `Encoded Message: ${info.encodedMessage}` : ""}
Encoding Format: ${info.encodingFormat}
Raw Inscription Data: ${info.rawInscriptionData}
`);
await confirm({ message: "Proceed?" }, { clearPromptOnDone: true });
};
export const broadcastBtcTransaction = async (
txHex: string,
api: string
): Promise<string> => {
const { data } = await axios.post<string>(`${api}/tx`, txHex, {
headers: { "Content-Type": "text/plain" },
});
return data;
};
export const addCommonOptions = (command: Command) => {
return command
.option(
"--bitcoin-api <url>",
"Bitcoin API",
"https://mempool.space/signet/api"
)
.option(
"--gas-price-api <url>",
"ZetaChain API",
"https://zetachain-athens.blockpi.network/lcd/v1/public/zeta-chain/crosschain/gasPrice/18333"
)
.addOption(
new Option("--private-key <key>", "Bitcoin private key").conflicts([
"name",
])
)
.addOption(
new Option("--name <name>", "Account name")
.default(DEFAULT_ACCOUNT_NAME)
.conflicts(["private-key"])
);
};
setupBitcoinKeyPair
: Initializes Bitcoin key pair from private key or account name, using Signet network.fetchUtxos
: Fetches UTXOs for a Bitcoin address from a specified API.displayAndConfirmTransaction
: Prints transaction details and prompts user confirmation.broadcastBtcTransaction
: Broadcasts raw Bitcoin transaction hex to the specified API.addCommonOptions
: Adds common CLI options like bitcoin-api, gas-price-api, private-key, and name.
From utils/bitcoin.helpers.ts
(lines 323-334, 98-186, 196-221, 236-279)
export const safeParseBitcoinAmount = (
amount: string,
decimals: number = 8
): number => {
const parsedAmount = ethers.parseUnits(amount, decimals);
if (parsedAmount > BigInt(Number.MAX_SAFE_INTEGER)) {
throw new Error("Amount exceeds maximum safe integer limit");
}
return Number(parsedAmount);
};
export const makeCommitTransaction = async (
key: bitcoin.Signer,
utxos: BtcUtxo[],
changeAddress: string,
inscriptionData: Buffer,
api: string,
amount: number,
feeSat = BITCOIN_FEES.DEFAULT_COMMIT_FEE_SAT
) => {
const scriptItems = [
key.publicKey.slice(1, 33),
bitcoin.opcodes.OP_CHECKSIG,
bitcoin.opcodes.OP_FALSE,
bitcoin.opcodes.OP_IF,
];
// Add inscription data in chunks if it exceeds 520 bytes (max script element size)
const MAX_SCRIPT_ELEMENT_SIZE = 520;
if (inscriptionData.length > MAX_SCRIPT_ELEMENT_SIZE) {
for (let i = 0; i < inscriptionData.length; i += MAX_SCRIPT_ELEMENT_SIZE) {
const end = Math.min(i + MAX_SCRIPT_ELEMENT_SIZE, inscriptionData.length);
scriptItems.push(inscriptionData.slice(i, end));
}
} else {
scriptItems.push(inscriptionData);
}
scriptItems.push(bitcoin.opcodes.OP_ENDIF);
const leafScript = bitcoin.script.compile(scriptItems);
/* p2tr */
const { output: commitScript, witness } = bitcoin.payments.p2tr({
internalPubkey: key.publicKey.slice(1, 33),
network: SIGNET,
redeem: { output: leafScript, redeemVersion: LEAF_VERSION_TAPSCRIPT },
scriptTree: { output: leafScript },
});
if (!witness) throw new Error("taproot build failed");
const { revealFee, vsize } = calculateRevealFee(
{
controlBlock: witness[witness.length - 1],
internalKey: key.publicKey.slice(1, 33),
leafScript,
},
BITCOIN_FEES.DEFAULT_REVEAL_FEE_RATE
);
const depositFee = Math.ceil((68 * 2 * revealFee) / vsize);
const amountSat = amount + revealFee + depositFee;
/* pick utxos */
utxos.sort((a, b) => a.value - b.value);
let inTotal = 0;
const picks: BtcUtxo[] = [];
for (const u of utxos) {
inTotal += u.value;
picks.push(u);
if (inTotal >= amountSat + feeSat) break;
}
if (inTotal < amountSat + feeSat) throw new Error("Not enough funds");
const changeSat = inTotal - amountSat - feeSat;
if (!commitScript) throw new Error("taproot build failed");
const psbt = new bitcoin.Psbt({ network: SIGNET });
psbt.addOutput({ script: commitScript, value: amountSat });
if (changeSat > 0)
psbt.addOutput({ address: changeAddress, value: changeSat });
for (const u of picks) {
const tx = (await axios.get<BtcTxById>(`${api}/tx/${u.txid}`)).data;
psbt.addInput({
hash: u.txid,
index: u.vout,
witnessUtxo: {
script: Buffer.from(tx.vout[u.vout].scriptpubkey, "hex"),
value: u.value,
},
});
}
psbt.signAllInputs(key);
psbt.finalizeAllInputs();
return {
controlBlock: witness[witness.length - 1],
internalKey: key.publicKey.slice(1, 33),
leafScript,
txHex: psbt.extractTransaction().toHex(),
};
};
export const calculateRevealFee = (
commitData: { controlBlock: Buffer; internalKey: Buffer; leafScript: Buffer },
feeRate: number
) => {
const witness = buildRevealWitness(
commitData.leafScript,
commitData.controlBlock
);
const txOverhead = BITCOIN_TX.TX_OVERHEAD; // 10 bytes: version (4) + marker (1) + flag (1) + locktime (4)
// Input vbytes:
// 36 bytes: outpoint (32-byte txid + 4-byte vout)
// 1 byte: scriptSig length (always 0 for segwit, but still encoded as a 1-byte varint)
// 4 bytes: sequence
// witness length is counted in weight units, so we divide by 4 to convert to virtual bytes
const inputVbytes = 36 + 1 + 4 + Math.ceil(witness.length / 4);
const outputVbytes = BITCOIN_TX.P2WPKH_OUTPUT_VBYTES; // 31 bytes: 8 (value) + 1 (script length) + 22 (P2WPKH script)
const vsize = txOverhead + inputVbytes + outputVbytes;
const revealFee = Math.ceil(vsize * feeRate);
return { revealFee, vsize };
};
export const makeRevealTransaction = (
commitTxId: string,
commitVout: number,
commitValue: number,
to: string,
feeRate: number,
commitData: { controlBlock: Buffer; internalKey: Buffer; leafScript: Buffer },
key: bitcoin.Signer
) => {
const psbt = new bitcoin.Psbt({ network: SIGNET });
const { output: commitScript } = bitcoin.payments.p2tr({
internalPubkey: commitData.internalKey,
network: SIGNET,
scriptTree: { output: commitData.leafScript },
});
psbt.addInput({
hash: commitTxId,
index: commitVout,
tapLeafScript: [
{
controlBlock: commitData.controlBlock,
leafVersion: LEAF_VERSION_TAPSCRIPT,
script: commitData.leafScript,
},
],
witnessUtxo: { script: commitScript!, value: commitValue },
});
const { revealFee } = calculateRevealFee(commitData, feeRate);
const outputValue = commitValue - revealFee;
if (outputValue < BITCOIN_LIMITS.DUST_THRESHOLD.P2WPKH) {
throw new Error(
`Insufficient value in commit output (${commitValue} sat) to cover reveal fee (${revealFee} sat) and maintain minimum output (${BITCOIN_LIMITS.DUST_THRESHOLD.P2WPKH} sat)`
);
}
psbt.addOutput({ address: to, value: outputValue });
psbt.signInput(0, key);
psbt.finalizeAllInputs();
return psbt.extractTransaction(true).toHex();
};
safeParseBitcoinAmount
: Parses BTC amount string to satoshis number safely.makeCommitTransaction
: Creates a commit transaction with inscription data, picks UTXOs, signs, and returns transaction hex and metadata.calculateRevealFee
: Calculates reveal transaction fee based on commit data and fee rate.makeRevealTransaction
: Creates and signs reveal transaction spending the commit output, ensuring dust threshold is respected.
From packages/commands/src/bitcoin/memo/depositAndCall.ts
(lines 69-90)
export const depositAndCallCommand = new Command()
.name("deposit-and-call")
.description("Deposit BTC and call a contract on ZetaChain")
.requiredOption("-r, --receiver <address>", "ZetaChain receiver address")
.requiredOption(
"-g, --gateway <address>",
"Bitcoin gateway (TSS) address",
"tb1qy9pqmk2pd9sv63g27jt8r657wy0d9ueeh0nqur"
)
.requiredOption("--amount <btcAmount>", "BTC amount to send (in BTC)")
.addOption(new Option("--data <data>", "Pass raw data"))
.option("--network-fee <fee>", "Network fee (in sats)", "1750")
.action(async (opts) => {
const validated = validateAndParseSchema(
opts,
memoDepositAndCallOptionsSchema,
{
exitOnError: true,
}
);
await main(validated);
});
- Shows a similar command definition for deposit-and-call but from a different path, useful for comparison of options and validation.
These snippets provide comprehensive context on:
- The command options schema and validation.
- Bitcoin key pair setup and UTXO fetching.
- Encoding of inscription data.
- Construction and broadcasting of commit and reveal transactions.
- Fee calculation and user confirmation.
This context is essential for reviewing the depositAndCall.ts
file's correctness, usage of utilities, and overall flow.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
gateway: string, | ||
key: bitcoin.Signer, | ||
amount: number, | ||
depositFee: number, | ||
networkFee: number, | ||
utxos: BtcUtxo[], | ||
changeAddr: string, | ||
api: string, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having these many arguments makes using functions hard to read, maintain, and test.
This could be refactored to something like:
interface BitcoinTxParams {
gateway: string;
key: bitcoin.Signer;
amount: number;
fee: number;
utxos: BtcUtxo[];
address: string;
api: string;
memo?: string;
}
const bitcoinMakeTransactionWithMemo = (params: BitcoinTxParams) => Promise<string>;
const gasPrice = response.data.GasPrice; | ||
const medianIndex = parseInt(gasPrice.median_index); | ||
const medianGasPrice = parseInt(gasPrice.prices[medianIndex]); | ||
return medianGasPrice * 68; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we save these magic numbers as constants?
|
||
if (memo.length < 20) throw new Error(errorNoReceiver); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same about these magic numbers
types/bitcoin.types.ts
Outdated
export const memoCallOptionsSchema = z.object({ | ||
bitcoinApi: z.string().url(), | ||
data: hexStringSchema.optional(), | ||
gasPriceApi: z.string().url(), | ||
gateway: z.string(), | ||
name: z.string().optional().default(DEFAULT_ACCOUNT_NAME), | ||
networkFee: z.string(), | ||
privateKey: z.string().optional(), | ||
receiver: z.string().optional(), | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like these memo schemas don't have the same validation rules as the inscription schemas (i.e.: name, pk exclusivity)
@fadeev Can you please add a memo deposit and call example command on the PR description? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few more comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is identical to the memo encode file (packages/commands/src/bitcoin/memo/encode.ts). There's no need to maintain two identical commands.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed encode
, because the memo doesn't have one. It was just copied accidentally. 8f56e29
const receiver = options.receiver?.startsWith("0x") | ||
? options.receiver.slice(2) | ||
: options.receiver; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've seen this logic repeated in a few different places, we should definitely create a very simple reusable helper.
types/bitcoin.types.ts
Outdated
name: z.string().optional().default(DEFAULT_ACCOUNT_NAME), | ||
networkFee: z.string(), | ||
privateKey: z.string().optional(), | ||
receiver: z.string().optional(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this always an evm compatible address? If that's the case we can reuse the evmAddressSchema
.
* | ||
* @param options - Command options including amounts, addresses, and contract parameters | ||
*/ | ||
const main = async (options: CallOptions) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should consider wrapping the await function calls with try/catch, otherwise users would probably see raw error stack traces instead of a helpful message.
@@ -63,37 +68,72 @@ const main = async (options: DepositAndCallOptions) => { | |||
); | |||
} | |||
|
|||
const amount = | |||
BITCOIN_LIMITS.MIN_COMMIT_AMOUNT + BITCOIN_LIMITS.ESTIMATED_REVEAL_FEE; | |||
const amount = Number(ethers.parseUnits(options.amount, 8)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fadeev This one seems important.
const data = options.data?.startsWith("0x") | ||
? options.data.slice(2) | ||
: options.data; | ||
|
||
const receiver = options.receiver?.startsWith("0x") | ||
? options.receiver.slice(2) | ||
: options.receiver; | ||
|
||
const memo = receiver + (data || ""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fadeev This one also seems important.
Deposit Amount: ${amount} sats (${(amount / 100000000).toFixed(8)} BTC) | ||
Network Fee: ${networkFee} sats (${(networkFee / 100000000).toFixed(8)} BTC) | ||
Deposit Fee: ${depositFee} sats (${(depositFee / 100000000).toFixed(8)} BTC) | ||
Deposit Total: ${amount + depositFee} sats (${( | ||
amount / 100000000 + | ||
depositFee / 100000000 | ||
).toFixed(8)} BTC) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like we're repeating this number a lot, we should extract it as a constant.
* @returns The calculated deposit fee in satoshis | ||
*/ | ||
export const calculateDepositFee = (txFee: number, txVsize: number): number => { | ||
return Math.ceil((txFee / txVsize) * 68 * 2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are these numbers? I see the number 68
used in a lot of different places, it seems like it's a very important number in the context of Bitcoin, probably worth saving in a constant with a descriptive name.
|
||
const memo = receiver || ""; | ||
|
||
const amount = Number(ethers.parseUnits(options.amount, 8)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we use the parseAmount
helper located on utils/bitcoin.command.helpers.ts:242?
Added memo deposit and call example. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested ACK ✅, all provided examples work as expected. Let's still ensure to address all review comments and suggestions to make it easier to maintain in the long run.
Memo
Deposit
https://mempool.space/signet/tx/c8ce3984d00afe9f77a805656b3e7e180ccf0a35374aa836dc14e4c7f47eb7b1
https://zetachain-athens.blockpi.network/lcd/v1/public/zeta-chain/crosschain/inboundHashToCctxData/c8ce3984d00afe9f77a805656b3e7e180ccf0a35374aa836dc14e4c7f47eb7b1
Deposit and Call
https://mempool.space/signet/tx/a88b164dc3969e1cab621bb1e043fa81c10c0d1d25573c2df9e9e8d4487f8a14
https://zetachain-athens.blockpi.network/lcd/v1/public/zeta-chain/crosschain/inboundHashToCctxData/a88b164dc3969e1cab621bb1e043fa81c10c0d1d25573c2df9e9e8d4487f8a14
Call
https://mempool.space/signet/tx/9158d94c86c380da83e0caf9280344938f2243a58630b2c47d789d5e331ca12d
https://zetachain-athens.blockpi.network/lcd/v1/public/zeta-chain/crosschain/inboundHashToCctxData/9158d94c86c380da83e0caf9280344938f2243a58630b2c47d789d5e331ca12d
Inscription
Deposit
https://mempool.space/signet/tx/b4394382fd7ae2199dedbe38ce75ab2fa492b96705be1d101791014386005abd
https://mempool.space/signet/tx/7a4fe80e9d49ad7cc5d3940db89371cac1bdd5dde0a8cb732fa0d723874b9750
https://zetachain-athens.blockpi.network/lcd/v1/public/zeta-chain/crosschain/inboundHashToCctxData/7a4fe80e9d49ad7cc5d3940db89371cac1bdd5dde0a8cb732fa0d723874b9750
Deposit and Call
https://mempool.space/signet/tx/f93609decc7e1d20116736c745ef817f7479a1cf850136972f5b5914c9aa0623
https://mempool.space/signet/tx/515d1437ab419637ff080258715400743cdf2e7a6260140ebcafe65c63066c56
https://zetachain-athens.blockpi.network/lcd/v1/public/zeta-chain/crosschain/inboundHashToCctxData/515d1437ab419637ff080258715400743cdf2e7a6260140ebcafe65c63066c56
Call
https://mempool.space/signet/tx/e45b31b2b092df0face2e304c21e817a683e620891d53428ace7633a749e8e5a
https://mempool.space/signet/tx/46d9f14d6595b4ef9939ff2d8f3d9405d02e96a84be4c8083f8fb214a8feebc3
https://zetachain-athens.blockpi.network/lcd/v1/public/zeta-chain/crosschain/inboundHashToCctxData/46d9f14d6595b4ef9939ff2d8f3d9405d02e96a84be4c8083f8fb214a8feebc3
Summary by CodeRabbit
New Features
--bitcoin-api
and--gas-price-api
for enhanced flexibility.Bug Fixes
Refactor