fix(zcash): update ZIP-317 fee calculation for NU6.1 compatibility#1671
fix(zcash): update ZIP-317 fee calculation for NU6.1 compatibility#1671Thorian1te merged 2 commits intomasterfrom
Conversation
NU6.1 (activated Nov 24 2025 at block 3146400) changed fee rules: - MARGINAL_FEE: 5000 → 10000 zatoshi (doubled) - GRACE_ACTIONS: 2 → 0 (removed, all actions must be paid) - Logical actions: inCount + outCount → max(inCount, outCount) Without this fix, transactions are rejected with: "-26: 66: tx unpaid action limit exceeded" Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughZcash fee calculation and fee-usage sites were changed: MARGINAL_FEE increased and calculateFee/getFee logic altered to base fees on max(inputCount, outputCount) with memo handling centralized in getFee; tests and client prepareMaxTx updated to match new fee values and call signatures. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/zcash-js/src/builder.ts`:
- Around line 21-25: The memo fee calculation is inconsistent across codepaths:
standardize on one output-count convention and ensure all fee computations use
the same helper; update calculateFee (and/or getFee) to define whether memo
consumes an extra output slot (use max(inCount, outCount) or max(inCount,
outCount + memoSlots) consistently), then change buildTx, buildMaxTx and any
callers (e.g., getFee usage in client code) to call that single helper
(calculateFee/getFee) rather than re-implementing logic so memo transfers,
selections, and builds all compute identical fees.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6cdd3e3c-fc1a-4baf-8a8e-93f7e36aac43
📒 Files selected for processing (2)
packages/xchain-zcash/__tests__/client.test.tspackages/zcash-js/src/builder.ts
| const MARGINAL_FEE = 10000 | ||
|
|
||
| function calculateFee(inCount: number, outCount: number): number { | ||
| const logicalActions = inCount + outCount | ||
| return MARGINAL_FEE * Math.max(GRACE_ACTIONS, logicalActions) | ||
| const logicalActions = Math.max(inCount, outCount) | ||
| return MARGINAL_FEE * Math.max(1, logicalActions) |
There was a problem hiding this comment.
Memo fee counting still diverges across estimate, selection, and build paths.
After switching to max(inCount, outCount), this becomes a user-visible mismatch: Line 56 and packages/xchain-zcash/src/client.ts, Lines 195-208, use getFee(..., 2, memo), buildTx() uses getFee(..., 3, memo) on Lines 154-155, and buildMaxTx() on Lines 105-107 bypasses getFee() entirely. That means a memo transfer can be quoted/selected against one fee and finalized with another, while long-memo sweeps still ignore extra memo slots and can remain underfunded. Please normalize the output-count convention and route every memo path through the same helper before merging.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/zcash-js/src/builder.ts` around lines 21 - 25, The memo fee
calculation is inconsistent across codepaths: standardize on one output-count
convention and ensure all fee computations use the same helper; update
calculateFee (and/or getFee) to define whether memo consumes an extra output
slot (use max(inCount, outCount) or max(inCount, outCount + memoSlots)
consistently), then change buildTx, buildMaxTx and any callers (e.g., getFee
usage in client code) to call that single helper (calculateFee/getFee) rather
than re-implementing logic so memo transfers, selections, and builds all compute
identical fees.
getFee is now the single source of truth for memo output slot counting. Callers pass only non-memo outputs; getFee adds memo slots internally. Fixes double-counting in buildTx/prepareMaxTx and bypass in buildMaxTx. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/xchain-zcash/src/client.ts (1)
111-116:⚠️ Potential issue | 🟡 MinorThe placeholder
'memo'string ingetFeeFromUtxosdoesn't affect Zcash's current fee calculation, but is still incorrect.While the method receives
_data(compiled memo Buffer) and should derive memo length from it, it instead passes the literal string'memo'(4 chars) togetFee, which calculates slots based on string length.However, this doesn't cause issues in practice for Zcash because:
getFeesWithRates()is overridden to throw (line 187-189), so the parent class's call togetFeeFromUtxosnever executes- Zcash uses its own
getFees()method (line 195-208) which correctly passes actual memo strings (options?.memo)That said, the method should still be fixed to avoid confusion and support potential future use cases. Since the compiled memo Buffer is provided but its original string length is lost, you could estimate memo length from the Buffer size or refactor to pass the original memo string alongside the compiled version.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xchain-zcash/src/client.ts` around lines 111 - 116, getFeeFromUtxos currently passes the literal string 'memo' to getFee which is incorrect; instead derive the memo length from the compiled memo Buffer _data and pass a string of that length (or otherwise adapt to getFee's expected input) so slot calculation is correct. Update the getFeeFromUtxos implementation (function name: getFeeFromUtxos, and call site getFee) to check _data, compute memoLength = _data.length (or an estimated character length based on the Buffer size), and pass a string constructed to that length (e.g., repeated char) to getFee, or refactor to pass the memo length if getFee can accept it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/xchain-zcash/src/client.ts`:
- Around line 111-116: getFeeFromUtxos currently passes the literal string
'memo' to getFee which is incorrect; instead derive the memo length from the
compiled memo Buffer _data and pass a string of that length (or otherwise adapt
to getFee's expected input) so slot calculation is correct. Update the
getFeeFromUtxos implementation (function name: getFeeFromUtxos, and call site
getFee) to check _data, compute memoLength = _data.length (or an estimated
character length based on the Buffer size), and pass a string constructed to
that length (e.g., repeated char) to getFee, or refactor to pass the memo length
if getFee can accept it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7af5da1b-924b-4beb-b3ae-e1cd72ef1a10
📒 Files selected for processing (2)
packages/xchain-zcash/src/client.tspackages/zcash-js/src/builder.ts
2 participants
NU6.1 (activated Nov 24 2025 at block 3146400) changed fee rules:
Without this fix, transactions are rejected with:
"-26: 66: tx unpaid action limit exceeded"
Summary by CodeRabbit