WIP: ARCs, Relays, and Hubs - #183
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Security | 1 critical |
🟢 Metrics 385 complexity · 8 duplication
Metric Results Complexity 385 Duplication 8
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
📝 WalkthroughWalkthroughThe repository adds contract capability tokens, state attestations, conditional spend tiers, Taproot policy and withdrawal helpers, contract address integration, message and sidechain updates, audit fields, tests, and npm Git dependency configuration. ChangesContract runtime and integration
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant Contract
participant contractTaproot
participant BitcoinPSBT
Contract->>contractTaproot: Build Taproot policy
contractTaproot->>contractTaproot: Select active tier and leaf
contractTaproot->>BitcoinPSBT: Prepare withdrawal PSBT
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| # core/http/hub from GitHub need `allow-git=all` so nested git dep preparation | ||
| # (commit-SHA fetches) is not refused. Keep this file so the monorepo default | ||
| # matches Hub / http / app packages. | ||
| allow-git=all |
There was a problem hiding this comment.
Medium — Supply-chain: re-enables npm 12 git-dependency RCE path
allow-git=all opts this repo (and GitHub installs of it) out of npm 12’s default allow-git=none, which was added specifically to block install-time code execution via a git dependency’s .npmrc overriding the git binary (works even with --ignore-scripts).
Attack path: A compromised or malicious transitive dependency introduces a git+/github: URL → npm fetches it under all → that git checkout’s .npmrc can redirect git → arbitrary code at install time. @fabric/core currently declares no git deps, so all is broader than needed; root (or keeping the default and scoping the opt-in to Hub/http only) would preserve the boundary.
Same relaxation is also forced in package.json report:install (npm i --allow-git=all after wiping the lockfile), which further widens install-time trust during that script.
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@DEVELOPERS.md`:
- Line 26: Update the installation instructions in DEVELOPERS.md to require npm
12 or newer before running the documented install commands. Add an explicit npm
version check and upgrade step after selecting the package.json-compatible Node
version, or change the pinned Node release to one that bundles npm 12, while
preserving the existing allow-git=all guidance.
🪄 Autofix
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 Plus
Run ID: 445cacea-94ae-48f2-bd82-e578a7ed9c85
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
.npmrcCHANGELOG.mdDEVELOPERS.mdpackage.json
| # core/http/hub from GitHub need `allow-git=all` so nested git dep preparation | ||
| # (commit-SHA fetches) is not refused. Keep this file so the monorepo default | ||
| # matches Hub / http / app packages. | ||
| allow-git=all |
There was a problem hiding this comment.
Medium — Supply-chain: re-enables npm 12 git-dependency RCE path
allow-git=all still opts this repo out of npm 12’s default allow-git=none, which blocks install-time code execution via a git dependency’s .npmrc overriding the git binary (even under --ignore-scripts). Confirmed still present at HEAD 91f034254; CHANGELOG notes root was rejected for nested SHA fetches.
Attack path: Compromised or malicious transitive git+/github: dep → fetched under all → that checkout’s .npmrc redirects git → arbitrary code at install time. This package still declares no git deps, so all is broader than needed.
Same relaxation remains in package.json report:install (npm i --allow-git=all after wiping the lockfile), which further widens install-time trust on a lockfile-free resolve.
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (5)
functions/contractTierWhen.js (1)
41-54: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
allOf: []activates the tier.An empty
allOfarray passes the loop and returnstrue. The module documents fail-closed behavior for malformed input. Reject an empty predicate list.♻️ Proposed change
const allOf = Array.isArray(when.allOf) ? when.allOf : null; - if (!allOf) return false; + if (!allOf || !allOf.length) return false;🤖 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 `@functions/contractTierWhen.js` around lines 41 - 54, Update evaluateTierWhen so an allOf array must contain at least one predicate; return false for an empty list before iterating, while preserving the existing validation and evaluation behavior for non-empty arrays.types/contract.js (1)
342-345: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPrefer the explicit override over the stored setting.
ladderreadsthis.settings.spendLadderfirst. A caller that passesoverrides.spendLaddercannot replace a configured ladder, because the stored value wins. The rest of the method treatsoverridesas the higher-precedence source. Invert the order for consistency.♻️ Proposed change
- const ladder = this.settings.spendLadder || overrides.spendLadder || null; + const ladder = overrides.spendLadder || this.settings.spendLadder || null;🤖 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 `@types/contract.js` around lines 342 - 345, Update the ladder selection in the surrounding contract-building method to prioritize overrides.spendLadder before this.settings.spendLadder, while retaining null as the fallback and preserving the existing tap.buildContractTaproot call.functions/contractTaproot.js (3)
683-741: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument or remove the
opts.useLaddercondition.The JSDoc states that
{ failover: true }orpublisherpluscsvBlocksselects the ladder. The condition at Line 696 also requiresopts.useLadder, which the JSDoc does not mention. A caller that passespublisherandcsvBlocksalone silently receives the legacy single-leaf vault, which uses a different address. Align the code with the documented contract.🤖 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 `@functions/contractTaproot.js` around lines 683 - 741, The ladder-selection condition in buildFederationVaultFromPolicy should honor the documented publisher-plus-csvBlocks inputs without requiring the undocumented opts.useLadder flag. Remove that extra condition so failover or a valid publisher with positive csvBlocks and ladder settings selects the ladder, or update the JSDoc and contract if the flag is intentionally required.
616-629: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueCompile the leaves once.
compileLeaves(built.policy)runs at Line 616 and again at Line 625, andbuildContractTaprootalready compiled the same leaves. Each call re-parses every pubkey and recompiles every script. Reuse one array.♻️ Proposed change
- const leaf = compileLeaves(built.policy).find((l) => l.kind === 'spend' && l.id === tier.id); + const leaves = compileLeaves(built.policy); + const leaf = leaves.find((l) => l.kind === 'spend' && l.id === tier.id); if (!leaf) throw new Error('spend leaf missing'); @@ - leaves: compileLeaves(built.policy), + leaves,🤖 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 `@functions/contractTaproot.js` around lines 616 - 629, Reuse the already compiled leaves array in the surrounding build flow instead of calling compileLeaves(built.policy) twice. Update the leaf lookup and prepareLeafPsbt arguments in the code around prepareLeafPsbt, using the existing compiled-leaves symbol from buildContractTaproot when available; otherwise compile once into a local array and pass it to both consumers.
29-34: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMake the
testfabric alias explicit.
signet => networks.testnetis acceptable because bech32 addresses match.test => networks.regtestis ambiguous, though, sincetestis commonly used for Bitcoin testnet (tb1…) whileregtestproducesbcrt1…. Rename this alias (for example toregtest) or add an explicit Fabric config documentation note.🤖 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 `@functions/contractTaproot.js` around lines 29 - 34, Update networkForFabricName so the ambiguous "test" alias is no longer mapped to networks.regtest; remove it or replace it with an explicit documented regtest alias, while preserving the existing regtest, testnet, and signet mappings.
🤖 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 `@functions/contractCapability.js`:
- Around line 64-79: Update the unverified branch in the contract capability
validation function to require an explicit caller opt-in before decoding
parse-only tokens, and mark the returned payload as unverified so it cannot be
confused with the issuerKey-verified result. Preserve the existing signature
verification path and reject omitted opt-in requests rather than treating the
two-part format as authentication.
In `@functions/contractStateSigning.js`:
- Around line 79-97: Update verifyContractStateTip to validate threshold before
calling verifyFederationWitnessOnMessage, rejecting omitted, non-numeric, or
otherwise invalid values rather than allowing downstream normalization to
default to 1. Preserve the existing witness construction and verification flow
for valid thresholds.
In `@functions/contractTaproot.js`:
- Around line 79-94: Update normalizeLock’s CSV branch to reject block values
above 65535, returning null for any value outside the valid low-16-bit nSequence
range while preserving existing handling for positive in-range values.
- Around line 212-239: Update the tier validation loop around normalizeLock,
lockValue, and prevAfter so ordering comparisons are performed only when the
compared locks have the same lock type; otherwise reject the mixed ladder with a
clear error. Apply this to both the previous-tier after check and the
until-versus-after check, while preserving existing same-type ordering rules.
- Around line 59-73: Do not clamp thresholds to the number of unique keys. In
buildKOfNTapscript, validate the normalized threshold and throw when it exceeds
keys.length instead of lowering k; preserve the existing minimum-threshold
handling. Apply the same validation in normalizeContractSpendPolicy for thr so
duplicate-key policies cannot compile with a weaker quorum.
- Around line 525-591: Update prepareLeafPsbt to accept the after lock
descriptor, and when it identifies a cltv leaf, set the PSBT locktime to the
descriptor’s value and ensure the input sequence is non-final. Preserve the
existing sequence behavior for CSV and other leaves, while allowing the
resulting CLTV transaction to satisfy its lock requirement.
In `@functions/contractTierWhen.js`:
- Around line 30-33: Update statePathEq to require that both pred.path and
pred.value are present before comparing values; return false for malformed
predicates, including missing value or path, while preserving the existing
strict comparison for valid predicates.
- Around line 13-22: Update getPath so each path segment is accepted only when
it is an own property of the current object, rejecting inherited and
prototype-chain keys such as __proto__, constructor, and prototype; return
undefined immediately for invalid segments while preserving normal traversal of
own contract-state properties.
In `@types/federation.js`:
- Around line 295-316: Unify taproot policy derivation between Federation and
Contract: in types/federation.js lines 295-316, replace the duplicated
validators/threshold/publisher/network/csvBlocks derivation with the inherited
toTaprootContract call, supplying federation-specific overrides; in
types/contract.js lines 340-364, extract the shared derivation into a protected
helper used by both classes and select one consistent default network instead of
the current regtest/bitcoin discrepancy.
- Around line 304-306: Update the csvBlocks fallback in the surrounding
federation configuration logic so settings.timeout is never converted directly
into a block count. Prefer the existing tap.DEFAULT_CSV_BLOCKS when csvBlocks is
unset, unless a clearly defined duration-to-block conversion is already
available; preserve explicit settings.csvBlocks handling.
- Around line 307-309: Update the custom spend-ladder branch in the relevant
federation address method so tap.toAddress receives this.settings.network,
matching the synthesized branch’s network behavior. Ensure configured ladders
are converted using the federation’s network setting rather than relying on the
ladder’s embedded network.
In `@types/token.js`:
- Around line 100-102: Update the context selection in the Token signing flow to
distinguish an explicitly provided options.ctx of null from an absent option, so
null suppresses this.settings.ctx and omits payload.ctx. Add a regression test
covering new Token({ ctx: { contractId } }).toSignedString({ ctx: null }) and
assert the signed payload contains no ctx.
---
Nitpick comments:
In `@functions/contractTaproot.js`:
- Around line 683-741: The ladder-selection condition in
buildFederationVaultFromPolicy should honor the documented
publisher-plus-csvBlocks inputs without requiring the undocumented
opts.useLadder flag. Remove that extra condition so failover or a valid
publisher with positive csvBlocks and ladder settings selects the ladder, or
update the JSDoc and contract if the flag is intentionally required.
- Around line 616-629: Reuse the already compiled leaves array in the
surrounding build flow instead of calling compileLeaves(built.policy) twice.
Update the leaf lookup and prepareLeafPsbt arguments in the code around
prepareLeafPsbt, using the existing compiled-leaves symbol from
buildContractTaproot when available; otherwise compile once into a local array
and pass it to both consumers.
- Around line 29-34: Update networkForFabricName so the ambiguous "test" alias
is no longer mapped to networks.regtest; remove it or replace it with an
explicit documented regtest alias, while preserving the existing regtest,
testnet, and signet mappings.
In `@functions/contractTierWhen.js`:
- Around line 41-54: Update evaluateTierWhen so an allOf array must contain at
least one predicate; return false for an empty list before iterating, while
preserving the existing validation and evaluation behavior for non-empty arrays.
In `@types/contract.js`:
- Around line 342-345: Update the ladder selection in the surrounding
contract-building method to prioritize overrides.spendLadder before
this.settings.spendLadder, while retaining null as the fallback and preserving
the existing tap.buildContractTaproot call.
🪄 Autofix
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 Plus
Run ID: 8ad37535-1ff4-4939-9572-b2e7eb5b3125
⛔ Files ignored due to path filters (3)
docs/APPLICATION_NAMESPACES.mdis excluded by!docs/**docs/DISTRIBUTED_EXECUTION.mdis excluded by!docs/**reports/install.logis excluded by!**/*.log
📒 Files selected for processing (13)
functions/applicationNamespaces.jsfunctions/contractCapability.jsfunctions/contractStateSigning.jsfunctions/contractTaproot.jsfunctions/contractTierWhen.jstests/applicationNamespaces.unit.jstests/contractCapability.unit.jstests/contractStateSigning.unit.jstests/contractTaproot.unit.jstypes/contract.jstypes/federation.jstypes/peer.jstypes/token.js
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #183 +/- ##
==========================================
+ Coverage 86.70% 86.88% +0.18%
==========================================
Files 96 96
Lines 34285 34355 +70
Branches 1 1
==========================================
+ Hits 29727 29850 +123
+ Misses 4558 4505 -53 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| # core/http/hub from GitHub need `allow-git=all` so nested git dep preparation | ||
| # (commit-SHA fetches) is not refused. Keep this file so the monorepo default | ||
| # matches Hub / http / app packages. | ||
| allow-git=all |
There was a problem hiding this comment.
Medium — Supply-chain: re-enables npm 12 git-dependency RCE path
allow-git=all opts this repo (and GitHub installs of it) out of npm 12’s default allow-git=none, which blocks install-time code execution via a git dependency’s .npmrc overriding the git binary (works even with --ignore-scripts). Confirmed still present at HEAD 3a4d87ab.
Attack path: Compromised or malicious transitive git+/github: dependency → fetched under all → that checkout’s .npmrc redirects git → arbitrary code at install time. @fabric/core still declares no git deps, so all is broader than needed (.npmrc is not in the published files list, but applies to clones and github: installs).
Same relaxation remains in package.json report:install (npm i --allow-git=all after wiping the lockfile), which further widens install-time trust on a lockfile-free resolve.
| # core/http/hub from GitHub need `allow-git=all` so nested git dep preparation | ||
| # (commit-SHA fetches) is not refused. Keep this file so the monorepo default | ||
| # matches Hub / http / app packages. | ||
| allow-git=all |
There was a problem hiding this comment.
Medium — Supply-chain: re-enables npm 12 git-dependency RCE path
allow-git=all opts this repo (and GitHub installs of it) out of npm 12’s default allow-git=none, which blocks install-time code execution via a git dependency’s .npmrc overriding the git binary (works even with --ignore-scripts). Confirmed still present at HEAD b984c1f60.
Attack path: Compromised or malicious transitive git+/github: dependency → fetched under all → that checkout’s .npmrc redirects git → arbitrary code at install time. @fabric/core still declares no git deps, so all is broader than needed (.npmrc is not in the published files list, but applies to clones and github: installs).
Same relaxation remains in package.json report:install (npm i --allow-git=all after wiping the lockfile), which further widens install-time trust on a lockfile-free resolve.
| # core/http/hub from GitHub need `allow-git=all` so nested git dep preparation | ||
| # (commit-SHA fetches) is not refused. Keep this file so the monorepo default | ||
| # matches Hub / http / app packages. | ||
| allow-git=all |
There was a problem hiding this comment.
Medium — Supply-chain: re-enables npm 12 git-dependency RCE path
allow-git=all opts this repo (and GitHub installs of it) out of npm 12’s default allow-git=none, which blocks install-time code execution via a git dependency’s .npmrc overriding the git binary (works even with --ignore-scripts). Confirmed still present at HEAD 3fc4b6602.
Attack path: Compromised or malicious transitive git+/github: dependency → fetched under all → that checkout’s .npmrc redirects git → arbitrary code at install time. @fabric/core still declares no git deps, so all is broader than needed (.npmrc is not in the published files list, but applies to clones and github: installs).
Same relaxation remains in package.json report:install (npm i --allow-git=all after wiping the lockfile), which further widens install-time trust on a lockfile-free resolve.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit a97e752. Configure here.
| let sequence = opts.sequence != null ? Number(opts.sequence) : undefined; | ||
| let locktime; | ||
| if (afterLock && afterLock.type === 'csv') { | ||
| if (sequence == null) sequence = afterLock.value; |
There was a problem hiding this comment.
CSV input sequence mis-encoded
High Severity
In prepareLeafPsbt, CSV timelock tiers set the input sequence to the raw block count (e.g. 144). BIP68 requires enabling relative locktime (0x00400000) in nSequence with the block count in the low 16 bits, so prepared tier and decay-migration PSBTs will not satisfy OP_CHECKSEQUENCEVERIFY on broadcast.
Reviewed by Cursor Bugbot for commit a97e752. Configure here.
| let catalog; | ||
| try { | ||
| catalog = JSON.parse(fields.catalogCanonical); | ||
| } catch (err) { |
There was a problem hiding this comment.
Patches blocked by catalog guard
Medium Severity
applyRegistryUpdateFields rejects any update when catalogCanonical is missing or empty before it reads patchesCanonical. Multi-op sidechain patches that rely on an empty catalog placeholder (or decoded empty string with a non-empty patchesCanonical) fail with catalogCanonical required instead of applying the patch array.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit a97e752. Configure here.
There was a problem hiding this comment.
Security review
Re-validated prior automation findings against HEAD a97e7523a (diff 214909a44…a97e7523a). Latest commit adds optional patchesCanonical on SIDECHAIN_STATE_PATCH plus message decode defaults.
Open: 1 medium (supply-chain). Prior contract auth/crypto concerns (unverified capability parse, tip-threshold fail-open, silent k-of-n clamp, malformed statePathEq) remain addressed.
No additional medium+ vulnerabilities identified in the new ARC/registry path: applyRegistryUpdateFields still requires caller-supplied fields and optional path policy; federation/auth gates are outside this helper. Peer contract:message now attaches wire hex for audit — not a new trust boundary bypass.
Note: Slack summary could not be posted — no Slack action is configured for this automation.
Sent by Cursor Automation: Find vulnerabilities
| # core/http/hub from GitHub need `allow-git=all` so nested git dep preparation | ||
| # (commit-SHA fetches) is not refused. Keep this file so the monorepo default | ||
| # matches Hub / http / app packages. | ||
| allow-git=all |
There was a problem hiding this comment.
Medium — Supply-chain: re-enables npm 12 git-dependency RCE path
allow-git=all opts this repo (and GitHub installs of it) out of npm 12’s default allow-git=none, which blocks install-time code execution via a git dependency’s .npmrc overriding the git binary (works even with --ignore-scripts). Confirmed still present at HEAD a97e7523a.
Attack path: Compromised or malicious transitive git+/github: dependency → fetched under all → that checkout’s .npmrc redirects git → arbitrary code at install time. @fabric/core still declares no git deps, so all is broader than needed (.npmrc is not in the published files list, but applies to clones and github: installs).
Same relaxation remains in package.json report:install (npm i --allow-git=all after wiping the lockfile), which further widens install-time trust on a lockfile-free resolve.
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)
functions/documentRegistrySidechain.js (1)
123-125: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAllow patch-only updates.
Lines 124-125 reject an empty
catalogCanonicalbefore thepatchesCanonicalbranch runs. This conflicts with the patch-first design and forces patch senders to include unused catalog JSON. ValidatecatalogCanonicalonly after no patch sequence is supplied.Proposed fix
function applyRegistryUpdateFields (state, fields, policy = null) { - if (!fields || typeof fields.catalogCanonical !== 'string' || !fields.catalogCanonical) { - return { ok: false, error: 'catalogCanonical required' }; - } + if (!fields || typeof fields !== 'object') { + return { ok: false, error: 'fields required' }; + } // ... basis validation and patchesCanonical handling ... + if (typeof fields.catalogCanonical !== 'string' || !fields.catalogCanonical) { + return { ok: false, error: 'catalogCanonical required' }; + } let catalog;🤖 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 `@functions/documentRegistrySidechain.js` around lines 123 - 125, Update applyRegistryUpdateFields so the initial validation does not require catalogCanonical when a patchesCanonical sequence is supplied. Validate catalogCanonical only in the non-patch update path, while preserving the existing required-field error for updates without patchesCanonical.
🤖 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 `@tests/contractTaproot.unit.js`:
- Around line 302-314: Update normalizeLock to enforce the Bitcoin CLTV
threshold consistently: accept height values below 500000000 only when
represented as height, accept unix values at or above 500000000 only when
represented as unix, and reject mismatched representations. Add coverage in the
normalizeLock tests for height: 500000000 and unix: 499999999, ensuring
timelockMature and transaction locktime behavior remain aligned.
In `@types/contract.js`:
- Around line 340-345: Update the JSDoc link in the shared spend-policy inputs
comment to use the class-qualified Contract member reference for
toTaprootContract, ensuring API.md resolves the generated link to the Contract
method.
In `@types/message.js`:
- Around line 1226-1227: Update the default branch of the optional field type
handling in encodeBody/decodeBody to throw a TypeError for unsupported trailing
optional field types instead of returning null, matching the rejection behavior
for non-empty decodeBody inputs.
---
Outside diff comments:
In `@functions/documentRegistrySidechain.js`:
- Around line 123-125: Update applyRegistryUpdateFields so the initial
validation does not require catalogCanonical when a patchesCanonical sequence is
supplied. Validate catalogCanonical only in the non-patch update path, while
preserving the existing required-field error for updates without
patchesCanonical.
🪄 Autofix
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 Plus
Run ID: c04ede96-8598-4957-b53d-2b9cb99c6cc0
⛔ Files ignored due to path filters (2)
docs/DISTRIBUTED_EXECUTION.mdis excluded by!docs/**docs/MESSAGE_BODY.mdis excluded by!docs/**
📒 Files selected for processing (20)
API.mdDEVELOPERS.mdMESSAGES.mdfunctions/contractCapability.jsfunctions/contractStateSigning.jsfunctions/contractTaproot.jsfunctions/contractTierWhen.jsfunctions/documentRegistrySidechain.jsreports/TODO.txttests/contractCapability.unit.jstests/contractStateSigning.unit.jstests/contractTaproot.unit.jstests/contractTierWhen.unit.jstests/fabric.token.jstests/functions.documentRegistrySidechain.jstypes/contract.jstypes/federation.jstypes/message.jstypes/peer.jstypes/token.js
💤 Files with no reviewable changes (1)
- reports/TODO.txt
🚧 Files skipped from review as they are similar to previous changes (8)
- types/token.js
- DEVELOPERS.md
- types/peer.js
- functions/contractStateSigning.js
- functions/contractTierWhen.js
- tests/contractStateSigning.unit.js
- functions/contractCapability.js
- functions/contractTaproot.js
| it('timelockMature for csv and cltv', function () { | ||
| assert.strictEqual(timelockMature(null, { utxoAgeBlocks: 0 }), true); | ||
| assert.strictEqual(timelockMature({ type: 'csv', blocks: 10 }, { utxoAgeBlocks: 9 }), false); | ||
| assert.strictEqual(timelockMature({ type: 'csv', blocks: 10 }, { utxoAgeBlocks: 10 }), true); | ||
| assert.strictEqual(timelockMature({ type: 'cltv', height: 100 }, { tipHeight: 99 }), false); | ||
| assert.strictEqual(timelockMature({ type: 'cltv', height: 100 }, { tipHeight: 100 }), true); | ||
| }); | ||
|
|
||
| it('normalizeLock rejects CSV above 65535', function () { | ||
| assert.ok(normalizeLock({ type: 'csv', blocks: 65535 })); | ||
| assert.strictEqual(normalizeLock({ type: 'csv', blocks: 65536 }), null); | ||
| assert.strictEqual(normalizeLock({ type: 'csv', blocks: 0 }), null); | ||
| }); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Validate the CLTV locktime type boundary.
normalizeLock accepts unix values below 500000000 and height values at or above it. timelockMature then uses the field name, but OP_CHECKLOCKTIMEVERIFY uses the numeric locktime threshold. This can mark a tier active while its withdrawal transaction is not mineable, or reject a tier that Bitcoin considers mature. (bips.dev)
Reject mismatched values and add boundary tests for height: 500000000 and unix: 499999999.
Proposed fix
+const CLTV_TIMESTAMP_THRESHOLD = 500000000;
+const MAX_LOCKTIME = 0xffffffff;
+
function normalizeLock (raw) {
// ...
if (type === 'cltv') {
- const value = Math.max(0, Math.floor(Number(raw.height != null ? raw.height : raw.unix) || 0));
- if (!value) return null;
- return { type: 'cltv', value, height: raw.height != null ? value : undefined, unix: raw.unix != null ? value : undefined };
+ const hasHeight = raw.height != null;
+ const hasUnix = raw.unix != null;
+ if (hasHeight === hasUnix) return null;
+ const value = Math.floor(Number(hasHeight ? raw.height : raw.unix));
+ if (!Number.isFinite(value) || value < 1 || value > MAX_LOCKTIME) return null;
+ if ((hasHeight && value >= CLTV_TIMESTAMP_THRESHOLD) ||
+ (hasUnix && value < CLTV_TIMESTAMP_THRESHOLD)) return null;
+ return {
+ type: 'cltv',
+ value,
+ height: hasHeight ? value : undefined,
+ unix: hasUnix ? value : undefined
+ };
}
}🤖 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 `@tests/contractTaproot.unit.js` around lines 302 - 314, Update normalizeLock
to enforce the Bitcoin CLTV threshold consistently: accept height values below
500000000 only when represented as height, accept unix values at or above
500000000 only when represented as unix, and reject mismatched representations.
Add coverage in the normalizeLock tests for height: 500000000 and unix:
499999999, ensuring timelockMature and transaction locktime behavior remain
aligned.
| /** | ||
| * Shared spend-policy inputs for {@link #toTaprootContract}. | ||
| * Subclasses (e.g. Federation) may override to supply validators from state. | ||
| * @param {object} [overrides] | ||
| * @returns {object} | ||
| */ |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the generated API link.
Line 341 generates the invalid #toTaprootContract fragment in API.md. Use the class-qualified member link so the generated reference resolves to the Contract method.
Proposed fix
- * Shared spend-policy inputs for {`@link` `#toTaprootContract`}.
+ * Shared spend-policy inputs for {`@link` Contract#toTaprootContract}.📝 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.
| /** | |
| * Shared spend-policy inputs for {@link #toTaprootContract}. | |
| * Subclasses (e.g. Federation) may override to supply validators from state. | |
| * @param {object} [overrides] | |
| * @returns {object} | |
| */ | |
| /** | |
| * Shared spend-policy inputs for {`@link` Contract#toTaprootContract}. | |
| * Subclasses (e.g. Federation) may override to supply validators from state. | |
| * `@param` {object} [overrides] | |
| * `@returns` {object} | |
| */ |
🤖 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 `@types/contract.js` around lines 340 - 345, Update the JSDoc link in the
shared spend-policy inputs comment to use the class-qualified Contract member
reference for toTaprootContract, ensuring API.md resolves the generated link to
the Contract method.
Source: Linters/SAST tools
| default: | ||
| return null; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject unsupported optional field types.
Line 1226 accepts an unsupported trailing optional field as null. encodeBody and non-empty decodeBody inputs reject the same schema. Throw TypeError in this branch.
Proposed fix
case 'u64':
return 0n;
default:
- return null;
+ throw new TypeError(`unsupported field type: ${def.type}`);📝 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.
| default: | |
| return null; | |
| default: | |
| throw new TypeError(`unsupported field type: ${def.type}`); |
🤖 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 `@types/message.js` around lines 1226 - 1227, Update the default branch of the
optional field type handling in encodeBody/decodeBody to throw a TypeError for
unsupported trailing optional field types instead of returning null, matching
the rejection behavior for non-empty decodeBody inputs.
1 participant
Implement the complete user story for ARCs, Relays, and Hubs.
Summary by CodeRabbit
New Features
Documentation
Chores
Note
High Risk
Changes contract publish authorization, P2P document handling, and new Bitcoin Taproot spend paths—security-sensitive areas that affect funds and mesh trust.
Overview
Adds contract Taproot tooling (
contractTaproot, tierwhenpredicates) plusContract/FederationtoAddress/toTaprootContractso spend policies compile to deterministic P2TR ladders (failover, decay/migrate, PSBT prep). Introduces contract-scoped capability tokens (OP_CONTRACT_READ/OP_CONTRACT_SIGN), k-of-nContractStateTipsigning, and new sharedCONTRACT_MESSAGEbody types (group journal catch-up, capability grants, withdrawal request/witness).Peer / wire:
contract:messageevents now carrywireMessage/messageHexfor journal attach; contract publish/patch allow-lists are tightened (wire signer must match body authority arrays; publisher no longer auto-granted). Document private relay gains_sendPrivateRelayedDocumentRequest; peeled/relayed document requests stay observe-only via delivery options.SIDECHAIN_STATE_PATCHgains optionalpatchesCanonicalfor multi-op RFC6902 fidelity; messagedecodeBodydefaults missing optional fields.Chores: root
.npmrcallow-git=alland install reporting for npm 12+ GitHub consumers; developer docs note npm 12+ requirement.Reviewed by Cursor Bugbot for commit a97e752. Configure here.