chore: fix broadcast failures and tighten action prompts#29
Conversation
…ng chainId in payload
📝 WalkthroughWalkthroughThis PR adds a new ChangesEVM enrichment and submission resilience
Sequence Diagram(s)sequenceDiagram
participant User
participant BorrowRecipe
participant EnrichEvmPayload
participant Provider as JsonRpcProvider
participant API
participant BroadcastSignedTx
User->>BorrowRecipe: submit signed transaction request
BorrowRecipe->>EnrichEvmPayload: enrich payload (chainId, nonce, fees)
EnrichEvmPayload->>Provider: fetch live nonce/fee data
Provider-->>EnrichEvmPayload: nonce, fee values
EnrichEvmPayload-->>BorrowRecipe: enriched payload
BorrowRecipe->>API: POST /submit signed tx
alt broadcast failure
API-->>BorrowRecipe: 5xx broadcast error
BorrowRecipe->>BroadcastSignedTx: broadcast(signedTx, network)
BroadcastSignedTx->>Provider: broadcastTransaction(signedTx)
Provider-->>BroadcastSignedTx: transactionHash
BroadcastSignedTx-->>BorrowRecipe: transactionHash
BorrowRecipe->>API: resubmit with transactionHash
else success
API-->>BorrowRecipe: confirmation
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
recipes/borrow.ts (1)
407-452: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winLet optional object arguments be skipped before recursing.
Line 407 tells users optional fields can be skipped, but the object branch always prompts nested fields and assigns
result[name]. Optional object args can still be sent as{}instead of being omitted.Suggested fix
} else if (type === "object" && prop.properties) { + if (!isRequired) { + const response: any = await Enquirer.prompt({ + type: "select", + name: "value", + message, + choices: ["<skip>", "configure"], + initial: "<skip>", + } as any); + if (response.value === "<skip>") continue; + } console.log(`\n${prop.label || name}:`); - result[name] = await promptFromSchema(prop as ArgumentSchemaDto, []); + const nested = await promptFromSchema(prop as ArgumentSchemaDto, []); + if (!isRequired && Object.keys(nested).length === 0) continue; + result[name] = nested;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@recipes/borrow.ts` around lines 407 - 452, Optional object arguments are still being recursed into and assigned even when the user should be able to skip them. Update the object-handling branch in promptFromSchema (the path that checks type === "object" && prop.properties) to mirror the skip behavior used for enums and booleans: prompt for an explicit skip choice when isRequired is false, and only call promptFromSchema and set result[name] when the user does not skip.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@recipes/borrow.ts`:
- Around line 619-646: Update the fee enrichment logic in borrow.ts so it
decides completeness based on the transaction type instead of treating any
non-zero gasPrice as sufficient. In the fee selection block around
hasLegacyFee/hasEip1559Fees, use payload.type (via parseIntegerField) to
determine whether to require gasPrice for legacy txs or
maxFeePerGas/maxPriorityFeePerGas for type-2 txs, and allow a type-2 payload
with only gasPrice to still fetch and fill EIP-1559 fields. Keep the existing
provider.getFeeData() fallback and ensure payload.type is normalized
consistently when setting fees.
- Around line 458-463: The required-array handling in the prompt flow should not
silently skip empty input when `response.value` is falsy. Update the logic
around the `initial` prompt result and the `if (!response.value)` branch so that
required arrays are explicitly validated in the `borrow.ts` flow, and either
reprompt or reject empty input before continuing. Make the fix in the code path
that assigns `result[name]` so required array arguments cannot be omitted by
pressing Enter.
In `@utils/evm-nonce.ts`:
- Around line 109-115: The already-known/already-in-mempool branch in the nonce
handling logic only returns a hash when the RPC message includes one, which
makes broadcastSignedTx() throw and breaks the fallback path. Update the branch
in evm-nonce.ts to derive and return the transaction hash from
Transaction.from(signedTx).hash whenever the message matches already known or
already in mempool, and keep the nonce too low case handled in its own separate
path.
---
Outside diff comments:
In `@recipes/borrow.ts`:
- Around line 407-452: Optional object arguments are still being recursed into
and assigned even when the user should be able to skip them. Update the
object-handling branch in promptFromSchema (the path that checks type ===
"object" && prop.properties) to mirror the skip behavior used for enums and
booleans: prompt for an explicit skip choice when isRequired is false, and only
call promptFromSchema and set result[name] when the user does not skip.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d98fb810-467d-4e7d-ab1f-c01e6ccd7e94
📒 Files selected for processing (2)
recipes/borrow.tsutils/evm-nonce.ts
| initial: isRequired && prop.default ? JSON.stringify(prop.default) : "", | ||
| } as any); | ||
|
|
||
| if (response.value) { | ||
| if (!response.value) { | ||
| if (!isRequired) continue; | ||
| } else { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Validate required array input before accepting it.
For a required array, pressing Enter takes the !response.value branch but does not set result[name] or reprompt, so the required argument is silently omitted.
Suggested fix
const response: any = await Enquirer.prompt({
type: "input",
name: "value",
message: `${message} (comma-separated or JSON array)`,
initial: isRequired && prop.default ? JSON.stringify(prop.default) : "",
+ validate: (input: string) => {
+ if (isRequired && input.trim() === "") return `${prop.label || name} is required`;
+ return true;
+ },
} as any);
- if (!response.value) {
+ if (!response.value.trim()) {
if (!isRequired) continue;
} else {📝 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.
| initial: isRequired && prop.default ? JSON.stringify(prop.default) : "", | |
| } as any); | |
| if (response.value) { | |
| if (!response.value) { | |
| if (!isRequired) continue; | |
| } else { | |
| initial: isRequired && prop.default ? JSON.stringify(prop.default) : "", | |
| validate: (input: string) => { | |
| if (isRequired && input.trim() === "") return `${prop.label || name} is required`; | |
| return true; | |
| }, | |
| } as any); | |
| if (!response.value.trim()) { | |
| if (!isRequired) continue; | |
| } else { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@recipes/borrow.ts` around lines 458 - 463, The required-array handling in the
prompt flow should not silently skip empty input when `response.value` is falsy.
Update the logic around the `initial` prompt result and the `if
(!response.value)` branch so that required arrays are explicitly validated in
the `borrow.ts` flow, and either reprompt or reject empty input before
continuing. Make the fix in the code path that assigns `result[name]` so
required array arguments cannot be omitted by pressing Enter.
| const hasLegacyFee = !isZeroish(payload.gasPrice); | ||
| const hasEip1559Fees = | ||
| !isZeroish(payload.maxFeePerGas) && !isZeroish(payload.maxPriorityFeePerGas); | ||
| if (!hasLegacyFee && !hasEip1559Fees) { | ||
| try { | ||
| const feeData = await provider.getFeeData(); | ||
| const apiType = parseIntegerField(payload.type); | ||
| const canUseEip1559 = feeData.maxFeePerGas !== null && feeData.maxPriorityFeePerGas !== null; | ||
| const wantEip1559 = apiType === 2 || (apiType === undefined && canUseEip1559); | ||
| if (wantEip1559 && feeData.maxFeePerGas && feeData.maxPriorityFeePerGas) { | ||
| payload.maxFeePerGas = feeData.maxFeePerGas.toString(); | ||
| payload.maxPriorityFeePerGas = feeData.maxPriorityFeePerGas.toString(); | ||
| payload.type = 2; | ||
| payload.gasPrice = undefined; | ||
| console.log( | ||
| ` Setting fees (from chain): maxFeePerGas=${feeData.maxFeePerGas}, maxPriorityFeePerGas=${feeData.maxPriorityFeePerGas}`, | ||
| ); | ||
| } else if (feeData.gasPrice) { | ||
| payload.gasPrice = feeData.gasPrice.toString(); | ||
| payload.type = 0; | ||
| payload.maxFeePerGas = undefined; | ||
| payload.maxPriorityFeePerGas = undefined; | ||
| console.log(` Setting fees (from chain): gasPrice=${feeData.gasPrice}`); | ||
| } | ||
| } catch (err: any) { | ||
| console.warn(` Could not fetch fee data: ${err?.message || err}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Decide missing fee fields based on transaction type.
Line 619 lets any non-zero gasPrice suppress fee enrichment, even when payload.type is 2. A type-2 payload with gasPrice but missing maxFeePerGas / maxPriorityFeePerGas remains under-filled.
Suggested fix
+ const apiType = parseIntegerField(payload.type);
const hasLegacyFee = !isZeroish(payload.gasPrice);
const hasEip1559Fees =
!isZeroish(payload.maxFeePerGas) && !isZeroish(payload.maxPriorityFeePerGas);
- if (!hasLegacyFee && !hasEip1559Fees) {
+ const missingRequiredFees =
+ apiType === 2
+ ? !hasEip1559Fees
+ : apiType === 0
+ ? !hasLegacyFee
+ : !hasLegacyFee && !hasEip1559Fees;
+
+ if (missingRequiredFees) {
try {
const feeData = await provider.getFeeData();
- const apiType = parseIntegerField(payload.type);
const canUseEip1559 = feeData.maxFeePerGas !== null && feeData.maxPriorityFeePerGas !== null;📝 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 hasLegacyFee = !isZeroish(payload.gasPrice); | |
| const hasEip1559Fees = | |
| !isZeroish(payload.maxFeePerGas) && !isZeroish(payload.maxPriorityFeePerGas); | |
| if (!hasLegacyFee && !hasEip1559Fees) { | |
| try { | |
| const feeData = await provider.getFeeData(); | |
| const apiType = parseIntegerField(payload.type); | |
| const canUseEip1559 = feeData.maxFeePerGas !== null && feeData.maxPriorityFeePerGas !== null; | |
| const wantEip1559 = apiType === 2 || (apiType === undefined && canUseEip1559); | |
| if (wantEip1559 && feeData.maxFeePerGas && feeData.maxPriorityFeePerGas) { | |
| payload.maxFeePerGas = feeData.maxFeePerGas.toString(); | |
| payload.maxPriorityFeePerGas = feeData.maxPriorityFeePerGas.toString(); | |
| payload.type = 2; | |
| payload.gasPrice = undefined; | |
| console.log( | |
| ` Setting fees (from chain): maxFeePerGas=${feeData.maxFeePerGas}, maxPriorityFeePerGas=${feeData.maxPriorityFeePerGas}`, | |
| ); | |
| } else if (feeData.gasPrice) { | |
| payload.gasPrice = feeData.gasPrice.toString(); | |
| payload.type = 0; | |
| payload.maxFeePerGas = undefined; | |
| payload.maxPriorityFeePerGas = undefined; | |
| console.log(` Setting fees (from chain): gasPrice=${feeData.gasPrice}`); | |
| } | |
| } catch (err: any) { | |
| console.warn(` Could not fetch fee data: ${err?.message || err}`); | |
| } | |
| } | |
| const apiType = parseIntegerField(payload.type); | |
| const hasLegacyFee = !isZeroish(payload.gasPrice); | |
| const hasEip1559Fees = | |
| !isZeroish(payload.maxFeePerGas) && !isZeroish(payload.maxPriorityFeePerGas); | |
| const missingRequiredFees = | |
| apiType === 2 | |
| ? !hasEip1559Fees | |
| : apiType === 0 | |
| ? !hasLegacyFee | |
| : !hasLegacyFee && !hasEip1559Fees; | |
| if (missingRequiredFees) { | |
| try { | |
| const feeData = await provider.getFeeData(); | |
| const canUseEip1559 = feeData.maxFeePerGas !== null && feeData.maxPriorityFeePerGas !== null; | |
| const wantEip1559 = apiType === 2 || (apiType === undefined && canUseEip1559); | |
| if (wantEip1559 && feeData.maxFeePerGas && feeData.maxPriorityFeePerGas) { | |
| payload.maxFeePerGas = feeData.maxFeePerGas.toString(); | |
| payload.maxPriorityFeePerGas = feeData.maxPriorityFeePerGas.toString(); | |
| payload.type = 2; | |
| payload.gasPrice = undefined; | |
| console.log( | |
| ` Setting fees (from chain): maxFeePerGas=${feeData.maxFeePerGas}, maxPriorityFeePerGas=${feeData.maxPriorityFeePerGas}`, | |
| ); | |
| } else if (feeData.gasPrice) { | |
| payload.gasPrice = feeData.gasPrice.toString(); | |
| payload.type = 0; | |
| payload.maxFeePerGas = undefined; | |
| payload.maxPriorityFeePerGas = undefined; | |
| console.log(` Setting fees (from chain): gasPrice=${feeData.gasPrice}`); | |
| } | |
| } catch (err: any) { | |
| console.warn(` Could not fetch fee data: ${err?.message || err}`); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@recipes/borrow.ts` around lines 619 - 646, Update the fee enrichment logic in
borrow.ts so it decides completeness based on the transaction type instead of
treating any non-zero gasPrice as sufficient. In the fee selection block around
hasLegacyFee/hasEip1559Fees, use payload.type (via parseIntegerField) to
determine whether to require gasPrice for legacy txs or
maxFeePerGas/maxPriorityFeePerGas for type-2 txs, and allow a type-2 payload
with only gasPrice to still fetch and fill EIP-1559 fields. Keep the existing
provider.getFeeData() fallback and ensure payload.type is normalized
consistently when setting fees.
| if (/already known|already in mempool|nonce.*too low|already exists/i.test(msg)) { | ||
| const hashMatch = msg.match(/0x[a-fA-F0-9]{64}/); | ||
| if (hashMatch) { | ||
| console.warn(` Tx already broadcast; reusing hash ${hashMatch[0]}`); | ||
| return hashMatch[0]; | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
ethers v6.13.4 Transaction.from serialized signed transaction hash property
💡 Result:
In ethers v6.13.4, the Transaction object (created via Transaction.from) contains a read-only hash property that represents the transaction hash [1][2]. When you initialize a Transaction object from a serialized signed transaction string using Transaction.from(serializedTx), the library automatically parses the transaction, verifies the signature, and computes the hash [3][4]. The hash property will contain the transaction hash as a string if the transaction is signed, or null if it is unsigned [1][2]. Key details regarding this property: - Access: It is a read-only property [1]. - Availability: It is computed automatically after parsing if the serialized transaction includes a valid signature [1][3]. - Behavior: You do not need to manually compute the hash; the Transaction object handles this internally once instantiated [3]. Example usage: const tx = ethers.Transaction.from(serializedSignedTransaction); console.log(tx.hash); // Returns the transaction hash string [1][3]
Citations:
- 1: https://docs.ethers.org/v6/api/transaction/
- 2: https://docs.ethers.org/v6-beta/api/transaction/
- 3: How do I get the from address of a signed transaction? ethers-io/ethers.js#820
- 4: TypeScript error: ethers.Transaction.from won't accept ethers.TransactionRequest ethers-io/ethers.js#4659
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file and nearby code
file="utils/evm-nonce.ts"
if [ ! -f "$file" ]; then
echo "Missing file: $file" >&2
exit 1
fi
wc -l "$file"
sed -n '1,220p' "$file"
# Find other references to the same error handling / hash recovery
rg -n "already known|already in mempool|nonce.*too low|already exists|Transaction.from|signedTx|reuse hash|already broadcast" .Repository: stakekit/api-recipes
Length of output: 5773
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the call site and any downstream fallback behavior
sed -n '430,500p' recipes/yields.ts
# Look for other uses of broadcastSignedTx
rg -n "broadcastSignedTx\\(" .Repository: stakekit/api-recipes
Length of output: 2555
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '430,500p' recipes/yields.ts
rg -n "broadcastSignedTx\\(" .Repository: stakekit/api-recipes
Length of output: 2555
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the borrow flow around broadcastSignedTx usage and fallback behavior
sed -n '680,740p' recipes/borrow.ts
# Check whether any other code recovers the hash independently
rg -n "localHash|transactionHash|submitTransaction|broadcastSignedTx|Transaction.from" recipes utilsRepository: stakekit/api-recipes
Length of output: 4768
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '680,740p' recipes/borrow.ts
rg -n "localHash|transactionHash|submitTransaction|broadcastSignedTx|Transaction.from" recipes utilsRepository: stakekit/api-recipes
Length of output: 4768
Recover the hash when the RPC says the tx is already known. broadcastSignedTx() still throws if that error omits the hash, which breaks the local fallback in recipes/borrow.ts even though the signed tx is already in the mempool. Use Transaction.from(signedTx).hash in the already-known/already-in-mempool branch, and keep nonce too low handled separately.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@utils/evm-nonce.ts` around lines 109 - 115, The
already-known/already-in-mempool branch in the nonce handling logic only returns
a hash when the RPC message includes one, which makes broadcastSignedTx() throw
and breaks the fallback path. Update the branch in evm-nonce.ts to derive and
return the transaction hash from Transaction.from(signedTx).hash whenever the
message matches already known or already in mempool, and keep the nonce too low
case handled in its own separate path.
2 participants
Added
utils/evm-nonce.ts: cachedJsonRpcProviderper network (withRPC_URL_<NETWORK>env overrides), pending-nonce lookup, local broadcast, and a heuristic for detecting backend broadcast failures.enrichEvmPayloadin the borrow signing path: patcheschainId,nonce, and gas fees on the API-supplied payload when missing or zero, so the signed tx is broadcast-ready./submitreturns 5xx "broadcast failed", the recipe rebroadcasts via the configured RPC and registers the hash via/submit { transactionHash }.patternand rejects non-0x…input for*Addressfields before submission.Changed
<skip> / true / falseselect; optional enums include a<skip>choice.Summary by CodeRabbit
New Features
Bug Fixes