DEM-665: Gas Fee Separation (combined fork with osDenomination)#817
Merged
Conversation
Detailed markdown spec for an agentic AI to implement the gas fee separation feature (hard fork). Covers genesis config, fee distribution logic, burn address, RPC address tracking, 9-decimal refactor, TLSN special ops, and SDK changes with exact file paths and code snippets. https://claude.ai/code/session_01W2wsTPvPsPZUP9wHUM5Yhq
…aration (myc#88, DEM-665 P2)
DEM-665 hard fork for gas-fee separation rides on the same activationHeight
as osDenomination (combined chain wipe). This commit lays the type and
loader scaffolding for the new fork name without touching consumers yet —
the gate remains null-by-default so behavior is bit-identical to pre-P2.
Changes:
- src/forks/forkConfig.ts:
- Split ForkConfig into BaseForkConfig + per-fork variants
(OsDenominationConfig, GasFeeSeparationConfig) joined as a
discriminated union, plus a ForkConfigByName map type for
statically-typed per-fork narrowing.
- Add gasFeeSeparation to ForkName and DEFAULT_FORK_CONFIG with a
PLACEHOLDER_TREASURY_ADDRESS (`0x` + 64 zeros). The placeholder is
valid while activationHeight === null; the loader rejects it once a
real activation is scheduled.
- src/forks/loadForkConfig.ts:
- Per-fork validator dispatch via validateForkEntry switch. The
gasFeeSeparation validator enforces a strict-lowercase 0x+64-hex
treasuryAddress (PR #778 G-1/G-4 lesson on case mismatch) and
refuses to seal genesis with the placeholder zero treasury when a
non-null activationHeight is set.
- New writeForkConfig dispatcher narrows the union per fork name.
- primeFeeDistributionFromForkConfig populates
SharedState.feeDistribution with the consensus-fixed addresses
(burnAddress = code constant, treasuryAddress from fork payload).
Distribution percentages remain undefined until
loadNetworkParameters folds them in (P13).
- Export GAS_FEE_SEPARATION_BURN_ADDRESS constant. Authoritative home
moves to migrations/gasFeeSeparation.ts in P12; re-export keeps
callers stable.
- src/utilities/sharedState.ts:
- feeDistribution: FeeDistributionRuntime | null field with the
combined view (addresses fork-fixed, percentages governance-driven).
- forkConfig typed as ForkConfigByName (narrowed) so callers can read
forkConfig.gasFeeSeparation.treasuryAddress without runtime checks.
- src/forks/index.ts:
- Re-export new types and GAS_FEE_SEPARATION_BURN_ADDRESS /
PLACEHOLDER_TREASURY_ADDRESS constants.
- testing/forks/: 13 new test cases in loadForkConfig.test.ts covering
treasuryAddress validation (missing/non-string/mixed-case/short),
placeholder rejection when scheduled, feeDistribution priming
behavior, combined-fork scenario, and re-load preservation of
governance-folded percentage groups.
- testing/forks/*.test.ts (8 files): snapshot type updated from
Record<ForkName, ForkConfig> to ForkConfigByName so the narrowed
per-fork shape round-trips through test setup/teardown.
Test suite: 106 pass / 0 fail / 349 expect() calls across
testing/forks/. Typecheck clean except pre-existing L2PS breakage
inherited via stabilisation merge (not introduced here).
…#98, DEM-665 P12) Creates the two consensus-fixed GCR accounts (burn and treasury) that the post-fork fee-distribution logic writes to. Single job per activation: insert zero-balance rows at known addresses, persist a fork_state ledger entry for idempotency. No balance touching — that remains osDenomination's responsibility, and the chainBlocks hook orders osDenomination FIRST so all balances are in OS units by the time gasFeeSeparation creates fresh accounts. Files: - src/forks/migrations/gasFeeSeparation.ts (NEW): runGasFeeSeparation- Migration() + isGasFeeSeparationMigrationApplied() mirror the osDenomination shape. Atomic via caller EntityManager, idempotent via the fork_state row. Defence-in-depth address validation (lowercase 0x + 64 hex, reject burn-address-as-treasury) on top of the loader's checks. Raw SQL inserts via portable `placeholder()` helper so the same code runs against Postgres (production) and the sqlite test harness. - src/libs/blockchain/chainBlocks.ts: gasFeeSeparation activation hook inside insertBlock's transaction, immediately after osDenomination. Ordering documented in a comment. Reads treasury from getSharedState.forkConfig.gasFeeSeparation.treasuryAddress (hydrated by loadForkConfigFromGenesis in P2). - src/forks/index.ts: re-export new symbols. - testing/forks/migrations/gasFeeSeparation.test.ts (NEW): 16 tests covering constants alignment (BURN_ADDRESS matches loader mirror), idempotency (fresh DB, after-run, double-run rejection), account creation (burn+treasury at balance 0, result struct), pre-existence handling (don't overwrite seeded burn/treasury), fork_state row shape, defence-in-depth validation (non-string, malformed, mixed case, burn-as-treasury), and coexistence with an existing osDenomination fork_state row. Test suite: 122 pass / 0 fail / 376 expect() calls across testing/forks/ (16 new in migrations/gasFeeSeparation.test.ts; the existing 106 unchanged). Typecheck clean except pre-existing L2PS breakage inherited via stabilisation merge.
…ntity (myc#89, DEM-665 P3)
Post-fork the validating node sets rpc_address to its own signing
pubkey so the fee-distribution edits (P5) can route the rpc_fee
portion to the correct operator account. Pre-fork rows carry `null` —
the legacy lump-sum gas path has no rpc-routing notion.
Files:
- src/libs/blockchain/transaction.ts:
- Constructor seeds rpc_address: null on freshly-built Transaction
objects.
- toRawTransaction carries the field across via `rpcAddress`.
- fromRawTransaction normalises a missing field (undefined) to the
explicit `null` declared by TxFee.
- src/model/entities/Transactions.ts:
- rpcAddress varchar column, nullable. Matches the convention used
by the other hex-address columns (from, to,
from_ed25519_address).
- src/migrations/AddRpcAddressToTransactions.ts (NEW):
- Idempotent ADD COLUMN IF NOT EXISTS. `synchronize: true` already
handles new boots; this migration is checked in so production
deploys are deterministic.
- src/forks/serializerGate.ts:
- Post-fork canonical serializer passes rpc_address through
unchanged (`fee.rpc_address ?? null`). No numeric coercion —
plain varchar.
- src/libs/blockchain/chainGenesis.ts +
src/libs/blockchain/routines/validateTransaction.ts +
src/libs/l2ps/L2PSBatchAggregator.ts +
src/libs/utils/demostdlib/deriveMempoolOperation.ts +
src/features/InstantMessagingProtocol/signalingServer/signalingServer.ts:
Structurally-required `rpc_address: null` added to every TxFee
literal the SDK type now demands. Internal Operation.fees blocks
(genesis fees, defineGas gas operation) are non-routable and
carry `null`.
- testing/forks/*.test.ts (5 files):
Same `rpc_address: null` injection into TxFee test fixtures so the
suite typechecks under the new SDK shape.
SDK side: TxFee.rpc_address + RawTransaction.rpcAddress already
shipped via the local SDK overlay at node_modules/@kynesyslabs/demosdk
(P3 SDK commit landed in /Users/tcsenpai/kynesys/sdks). P9 will
publish 4.0.0-rc.1 and bump the package.json pin to drop the overlay.
Test suite: 122 pass / 0 fail in testing/forks/ (no regressions).
The pre-existing tests/governance/snapshotWeightIntegrity.test.ts
failure is unrelated to this commit (verified on a clean stash) and
tracked separately.
Typecheck clean except pre-existing L2PS breakage inherited via
stabilisation merge.
…, DEM-665 P4)
Adds the per-component fee calculator that post-fork code (P5/P6) reads
to drive the new fee-distribution edit generator. Returns
`{network_fee, rpc_fee, additional_fee, total}`. Does NOT read the
deprecated `burnFee` shared-state scalar — post-fork the burned share
comes out of the per-component distribution percentages
(networkFeeBurnPct, etc.) carried by NetworkParameters (P13 wires
those in; P8 retires the scalar).
The legacy `calculateCurrentGas` default export is kept on the
three-scalar shape (networkFee + rpcFee + burnFee) so pre-fork callers
— `determineGasForOperation`, `txToGCROperation`, and the dead-code
`defineGas` path — observe the exact same total they did before
DEM-665. The pre-fork → post-fork switch happens in P6 inside
confirmTransaction, gated by `isForkActive("gasFeeSeparation", ...)`.
Files:
- src/libs/blockchain/routines/calculateCurrentGas.ts:
- NEW `export interface FeeBreakdown`.
- NEW `export async function calculateFeeBreakdown(payload)` returning
the per-component split. Reads `getSharedState.networkFee`,
`rpcFee`; surge factor still 1 today via the same stub.
- `additional_fee` is always 0 — reserved for a future dApp-paid
fee path described in the DEM-665 spec.
- Legacy `calculateCurrentGas` and `calculateComposedGas` left
intact (marked @deprecated on the default export) so callers
that haven't migrated are bit-identical to pre-bump behavior.
- tests/governance/calculateCurrentGas.test.ts:
- Existing 3 legacy-flat-fee tests preserved unchanged.
- 5 new tests under "calculateFeeBreakdown — per-component split
(DEM-665)" covering: default returns 1/1/0/2, independent
component scaling (burnFee bump intentionally ignored),
total = sum-of-components invariant, zero-components yield
zero total, additional_fee always 0.
Test suite: 8 pass / 0 fail / 16 expect() calls in
tests/governance/calculateCurrentGas.test.ts; 122 pass / 0 fail in
testing/forks/ (no regressions).
…99, DEM-665 P13)
DEM-665 makes the per-component fee-distribution percentages
governable from day 1, with tighter safety bounds so a single
passing proposal cannot drain the treasury in one cycle. Treasury
and burn addresses remain immutable fork-payload (see P2).
Layer 1 (percentage cap):
- New `DISTRIBUTION_MAX_CHANGE_PERCENT = 10` constant.
- `withinPercentCap` takes the cap as a parameter; distribution keys
use the tighter ±10%, everything else keeps the historical ±50%.
Layer 2 (absolute floor/ceiling):
- NUMERIC_BOUNDS gains `additionalFee: [0, 5000]` and `*Pct:
[0, 100]` for every distribution percentage.
Layer 3 (NEW — cross-key sum-100):
- `checkDistributionSumInvariant` runs on the merged (current ⊕
proposed) view of each group:
network_fee: burnPct + treasuryPct === 100
additional_fee: burnPct + treasuryPct === 100
special_ops: burnPct + treasuryPct + rpcPct === 100
Untouched groups are skipped (they were valid by induction).
- First-failing key is returned in the BoundsCheck for diagnostic
attribution.
Governable set:
- PHASE_1_GOVERNABLE_KEYS extended with `additionalFee` and every
member of the new `DISTRIBUTION_KEYS` set.
Hardcoded fallback:
- `HARDCODED_FALLBACK_NETWORK_PARAMETERS` carries the SPEC defaults
(50/50, 25/75, 25/50/25) plus `additionalFee: 0`. Mirror exposed
through `getGenesisNetworkParameters()`.
SharedState mirror:
- `loadNetworkParameters` now folds the merged percentages onto
`getSharedState.feeDistribution`. Addresses (burnAddress,
treasuryAddress) primed earlier by `loadForkConfigFromGenesis` (P2)
are preserved — only the percentage groups are overwritten. A
governance proposal changing any *Pct takes effect on the next tx
without a node restart (P5's feeDistribution.ts dereferences this
at call time).
Tests:
- tests/governance/safetyBounds.test.ts gets a new
"DEM-665 distribution-percentage governance" describe block with
9 cases: additionalFee bounds, single-key sum-100 violation,
balanced two-key shift within cap, +12% rejection above the 10%
cap, ceiling rejection, balanced three-key special_ops
rebalance, special_ops merged-sum mismatch, untouched-group skip.
Test suite: 31 pass / 0 fail in tests/governance/safetyBounds.test.ts;
237 pass / 1 pre-existing fail (snapshotWeightIntegrity — unrelated
mock setup, tracked separately) in testing/forks/ + tests/governance/.
Typecheck clean except pre-existing L2PS breakage inherited via
stabilisation merge. SDK side committed in sister repo at
858e205 (NetworkParameters extension); local overlay applied for
unblocking — proper publish handled at P9.
… DEM-665 P5)
Post-fork, the validating node calls these helpers from
`validateTransaction.confirmTransaction` (P6) and the TLSN-handling
branch of `handleNativeOperations` (P7) to convert per-component fee
totals into a sequence of GCREditBalance entries:
- generateFeeDistributionEdits: regular tx. Emits the
network_fee (burn/treasury), rpc_fee (100% to rpc operator), and
additional_fee (burn/treasury) blocks in that order. Components
with 0 totals are skipped; an rpcAddress=null but rpcFee>0
situation logs a warning and skips the rpc block (P6 will never
leave rpcAddress null in production).
- generateSpecialOpsFeeEdits: TLSN. Splits a single total across
burn / rpc-operator / treasury per the SPEC default 25/50/25.
If the rpc operator address is missing, the unrouted share is
folded into treasury so the balance still closes.
Consensus-critical rounding: every per-recipient share is
Math.floor(total * pct / 100); treasury captures the remainder so the
sum of `add` amounts equals the `remove` amount exactly. Number math
is safe within OS magnitudes 2^53.
Reads `getSharedState.feeDistribution` (populated by
loadForkConfigFromGenesis at P2 for the addresses, and by
loadNetworkParameters at P13 for the percentage groups). Defensive
null guard returns [] + logs error — callers MUST gate on
isForkActive("gasFeeSeparation", height) upstream because the helper
has no block-height context.
Tests: tests/blockchain/feeDistribution.test.ts — 16 cases:
- generateFeeDistributionEdits: null guard, zero-component skip,
50/50 network split, 100% rpc routing, null-rpc skip, 25/75
additional split, rounding remainder to treasury, 0%-burn
branch, isRollback forwarding, block ordering invariant.
- generateSpecialOpsFeeEdits: null guard, totalFee=0, 4-edit SPEC
default, rounding remainder to treasury, null-rpc fallback
(rpc share folded into treasury), isRollback forwarding.
Test suite: 16/16 pass / 44 expect() in tests/blockchain/.
Regression: 253/254 pass across testing/forks/ + tests/governance/ +
tests/blockchain/ (the 1 fail is the pre-existing
snapshotWeightIntegrity mock-setup issue unrelated to DEM-665).
Typecheck clean except pre-existing L2PS breakage inherited via
stabilisation merge.
…#92, DEM-665 P6)
Post-fork, confirmTransaction now stamps the per-component fees +
rpc_address onto the tx, balance-checks the sender (PROD only), and
prepends the fee-distribution GCREdits onto tx.content.gcr_edits so
peers compute the same signed hash and apply the same balance moves.
The injection runs BEFORE signValidityData() so the appended edits
are part of the signed payload (otherwise peers would diverge).
Files:
- src/libs/blockchain/routines/validateTransaction.ts:
- Import calculateFeeBreakdown (from P4) + generateFeeDistribution-
Edits (from P5) + isForkActive (from forks barrel).
- New `applyGasFeeSeparation(tx, validityData)` helper:
1. Coerces sender pubkey via forgeToHex (same as defineGas).
2. Calls calculateFeeBreakdown — refuses to proceed on a
non-integer / negative total.
3. Resolves this node's signing pubkey + stamps
transaction_fee.{network_fee, rpc_fee, additional_fee,
rpc_address}.
4. PROD-only: reads GCR.getGCRNativeBalance and rejects if
senderBalance < total.
5. Calls generateFeeDistributionEdits and prepends the
returned GCREdits onto tx.content.gcr_edits.
- Gating: invoked only inside
`if (isForkActive("gasFeeSeparation", referenceBlock))`. Pre-fork
behaviour is unchanged — the legacy defineGas() path remains the
no-op placeholder it has been since the original /* REVIEW */
block disabled the live caller.
- On failure (balance, breakdown sanity), validityData.valid is
set to false with `[Tx Validation] [FEE ERROR] …` message and
returned signed.
Test suite: 253/254 pass across testing/forks/ + tests/governance/ +
tests/blockchain/ (1 pre-existing snapshotWeightIntegrity mock
failure unrelated to this commit). Typecheck clean except
pre-existing L2PS breakage from stabilisation merge.
P6 is a wiring commit — the new code path is exercised by P10
integration tests (fork-boundary rehearsal scenario) once those
land. Unit-level coverage of the helpers it composes already exists
in tests/governance/calculateCurrentGas.test.ts (P4) and
tests/blockchain/feeDistribution.test.ts (P5).
… (myc#93, DEM-665 P7)
Post-fork the two TLSNotary native operations stop relying on the
legacy single-remove burn (where fees vanished from supply with no
recipient) and instead route the fee through the new 25/50/25
burn/rpc/treasury distribution via generateSpecialOpsFeeEdits.
Pre-fork the legacy single-remove path is preserved verbatim so
re-syncing a pre-fork chain stays bit-identical.
Files:
- src/libs/blockchain/gcr/gcr_routines/handleNativeOperations.ts:
- New ONE_DEM = 1_000_000_000 + getTlsnFees(blockHeight) helper:
post-fork scales TLSN_REQUEST_FEE / TLSN_STORE_BASE_FEE /
TLSN_STORE_PER_KB_FEE by ONE_DEM (1 DEM = 10^9 OS). Pre-fork
leaves them at the legacy "1 DEM = 1 unit" values.
- tlsn_request case: branches on isForkActive("gasFeeSeparation",
blockHeight). Post-fork builds specialOps edits with rpcAddress
pulled from tx.content.transaction_fee.rpc_address (stamped
upstream by P6). Pre-fork keeps the original single-remove
burnFeeEdit untouched.
- tlsn_store case: same fork-branch around the storage-fee edit.
Pre-fork single-remove preserved; post-fork specialOps edits
routed through generateSpecialOpsFeeEdits. The storage-size math
(base + perKB × proofSizeKB) is unchanged — only the
per-constant magnitudes scale post-fork.
Block height source: tx.blockNumber when present, else
getSharedState.lastBlockNumber, defaulting to 0 — same fallback
chain other call sites use.
When rpc_address is missing on a post-fork tx (defensive case —
shouldn't happen because P6 always stamps it), the rpc share is
folded into treasury inside generateSpecialOpsFeeEdits so the
balance still closes; a warning is logged.
Test suite: 253/254 pass across testing/forks/ + tests/governance/ +
tests/blockchain/ (the 1 fail is the pre-existing
snapshotWeightIntegrity mock-setup issue unrelated to DEM-665).
Typecheck clean except pre-existing L2PS breakage from
stabilisation merge.
Wiring commit — exercised end-to-end by P10 rehearsal scenario.
…4, DEM-665 P8)
Post-fork the burn account at `feeDistribution.burnAddress` becomes
consensus-significant: balances added to it represent permanently
removed supply. A normal `remove` against this pubkey would
re-circulate burned coins, defeating the burn-percentage routing
done by P5/P6/P7. This commit refuses such removes inside
GCRBalanceRoutines.apply.
Carve-outs (both intentional):
1. Rollback flow: GCRBalanceRoutines inverts add↔remove BEFORE the
guard runs. A rollback of a prior burn-`add` (which now reads
as `remove + isRollback=true`) IS allowed — the carve-out keeps
fee distribution reversible.
2. Pre-fork: the guard is wrapped in
`isForkActive("gasFeeSeparation", lastBlockNumber)`, so any
legacy code path that happens to remove from the zero address
pre-fork is unchanged.
Address comparison is case-normalised via toLowerCase() on both
sides — PR #778 G-1/G-4 lesson (myc#6).
Files:
- src/libs/blockchain/gcr/gcr_routines/GCRBalanceRoutines.ts:
- import isForkActive from "@/forks"
- New block placed AFTER the rollback inversion and BEFORE the
balance math. Reads getSharedState.feeDistribution +
getSharedState.lastBlockNumber. No-op when feeDistribution is
null or the fork is inactive.
- tests/blockchain/GCRBalanceRoutines.test.ts (NEW): 8 cases
covering:
• normal remove against burn rejected when fork active
• rollback inversion against burn allowed
• normal remove against burn allowed pre-fork
• remove against non-burn account allowed
• add to burn allowed (fee-distribution path)
• uppercase-hex edit account still hits the guard (case norm)
• null feeDistribution falls through (defensive)
• lastBlockNumber < activationHeight falls through (gate)
Test suite: 8/8 pass / 10 expect(). Regression: 253/254 pass across
testing/forks/ + tests/governance/ + tests/blockchain/ (the 1 fail
is the pre-existing snapshotWeightIntegrity mock-setup issue
unrelated to DEM-665). Typecheck clean except pre-existing L2PS
breakage from stabilisation merge.
…M-665 P10 partial)
Adds the P10-required handleNativeOperations TLSN test coverage —
the final unit-layer gap left in DEM-665.
Tests cover both branches of tlsn_request and tlsn_store:
Pre-fork (legacy path):
- tlsn_request emits a single 1-DEM remove (legacy burn-by-omission).
- tlsn_store emits a single size-scaled remove (storeBase + perKB).
Post-fork (new path):
- tlsn_request scales fee to 10^9 OS, emits 4 edits with the
SPEC default 25/50/25 burn/rpc/treasury split (250M / 500M / 250M).
- tlsn_request with rpcAddress=null folds rpc share into treasury
so the balance closes (3 edits: remove + burn + treasury 750M).
- tlsn_store scales size-based fee × 10^9 and routes through the
same special-ops split; sum invariant verified.
Mocks tokenManager so the test runs without a live token store —
token shape mirrors the real entity just enough for the handler's
status / owner / domain checks.
P10 status (full picture):
✅ feeDistribution.test.ts (16 tests, P5)
✅ GCRBalanceRoutines.test.ts (8 tests, P8)
✅ calculateCurrentGas.test.ts extended (5 tests, P4)
✅ safetyBounds.test.ts extended (9 tests, P13)
✅ governanceHandlers.test.ts implicit (GENESIS struct now carries
the new keys; existing equality assertion still passes)
✅ handleNativeOperations.test.ts (NEW — this commit)
⏭️ validateTransaction.test.ts — applyGasFeeSeparation is a
module-private helper. Direct unit coverage requires either
extracting it (architectural decision deferred) or mocking
Chain/GCR/forgeToHex/Transaction.confirmTx — heavy surface for
a wiring layer already exercised by the component tests above.
⏭️ testing/forks/rehearsal/scenarios/09-fee-distribution.ts +
10-burn-spend-rejection.ts — devnet integration scenarios
mirror the P5b rehearsal pattern (myc#22). They need a 4-node
Postgres devnet harness and a coordinated boundary-cross run,
so they are filed for the rehearsal team to run alongside the
existing osDenomination scenarios.
Test suite: 266/267 pass / 683 expect() calls across testing/forks/
+ tests/governance/ + tests/blockchain/. The 1 fail is the
pre-existing snapshotWeightIntegrity mock-setup issue unrelated to
DEM-665. Typecheck clean except pre-existing L2PS breakage from
stabilisation merge.
…5 P11) Runbook (decimal_planning/RUNBOOK_FORK_ACTIVATION.md §9): - New "gasFeeSeparation co-activation" section appended. - 9 subsections: bundling rationale, genesis.json delta + validation rules, treasury key custody (ops-owned), activation hook semantics, pre-flight checklist additions (jq one-liners for treasuryAddress format / activationHeight match / placeholder rejection), post-activation verification (getAddressInfo + fork_state SQL), rollback (atomic with osDenomination), governance-mutable percentages (±10% / sum-100 invariant), gasFeeSeparation-specific don't-do list. Plan doc (docs/GAS_FEE_SEPARATION_PLAN.md): - New "⚠️ As-shipped status (read first)" header at the top of the file calls out the deviations from the original spec: * fee_config does NOT ship at the top of genesis.json (chainGenesis.ts:60-73 hashes extra; verified) * treasuryAddress = fork-payload; burnAddress = code constant; distribution = governance-mutable * Distribution percentages governable from day 1 with tighter bounds (±10% per proposal, sum-100 cross-key invariant) * burnFee scalar retired - File:line table mapping each concern to its implementation site. - Test-suite inventory (7 suites, 113 DEM-665 tests). - Deferred items table (P9 publish, myc#100 integration scenarios, myc#101 applyGasFeeSeparation extraction). The original specification body below the as-shipped header is left unchanged so the historical record stays intact — readers see the deviation header first and can compare against the spec verbatim when needed. No code changes; no test impact. Closes the documentation portion of myc epic #10.
…5 P9) User published the DEM-665-companion SDK as 4.0.0 (final, not rc). Drops the local node_modules/@kynesyslabs/demosdk symlink overlay that was used to unblock node typecheck during P3..P13 development. Breaking changes in 4.0.0 (already absorbed in the DEM-665 commits P2..P13): - TxFee: + `rpc_address: string | null` - RawTransaction: + `rpcAddress: string | null` - NetworkParameters: + `additionalFee` and seven distribution- percentage fields (networkFeeBurnPct/TreasuryPct, additionalFeeBurnPct/TreasuryPct, specialOpsBurnPct/TreasuryPct/RpcPct); `burnFee` removed. Post-install verification: - node_modules/@kynesyslabs/demosdk/package.json reports version 4.0.0 - TxFee.d.ts carries `rpc_address` - NetworkParameters.d.ts carries the 8 new fields - Typecheck clean (except pre-existing L2PS breakage) - Test suite: 266 pass / 1 pre-existing fail / 683 expect() across testing/forks/ + tests/governance/ + tests/blockchain/ bun.lock is gitignored so the lockfile churn does not enter the commit, only the manifest pin moves.
Adds the devnet integration coverage for gasFeeSeparation
co-activation. Scenario 09 exercises the post-fork invariants;
scenario 10 is a placeholder pending tx-signing support in the
harness.
Files:
- testing/forks/rehearsal/genesis/genesis-fork-low-gasFee.json (NEW):
Mirrors genesis-fork-low.json but adds the gasFeeSeparation fork
entry at activationHeight=5 alongside osDenomination. Sentinel
treasury address 0xfeedface...feedface so scenario 09 can read it
back from gcr_main directly.
- testing/forks/rehearsal/lib/devnetControl.ts: new
GENESIS_FORK_LOW_GAS_FEE path constant.
- testing/forks/rehearsal/lib/nodeQueries.ts: two new helpers —
`getGasFeeForkStateRow` (gasFeeSeparation row in fork_state) and
`getGcrAccount` (single-pubkey lookup with numeric::text cast).
- testing/forks/rehearsal/lib/assertions.ts:
`assertGasFeeForkStateConvergence` (both nodes must have
applied=true with the same applied_at_block; sum/cap columns are
intentionally NULL on this row), and `assertGcrAccountConvergence`
(gcr_main row present at expected balance on every node).
- testing/forks/rehearsal/scenarios/09-fee-distribution.ts (NEW):
4 nodes cross combined fork at height 5, verify
* osDenomination still activates + sum invariant holds
(regression guard — gasFeeSeparation must not break decimals)
* gasFeeSeparation fork_state row present + converged
* burn account 0x000…000 exists with balance 0 on every node
* treasury account exists with balance 0 on every node
* liveness check — network advances 60s past activation
~187 lines, mirrors scenario 01's shape.
- testing/forks/rehearsal/scenarios/10-burn-spend-rejection.ts (NEW):
Documented placeholder. Devnet-level burn-spend rejection requires
a signed tx with a manual remove-from-burn GCREdit, which the
rehearsal harness cannot construct today (no signing helper for
genesis-funded accounts — same constraint that limits scenario 06
to read-only RPC). Coverage at unit level in
tests/blockchain/GCRBalanceRoutines.test.ts (8 cases); the
scenario file documents the deferral and outlines the body to add
once tx-signing infra lands.
- testing/forks/rehearsal/run-all.sh: scenarios 09 and 10 appended
to the runner order; header comment updated to reflect the
combined-fork DEM-665 cycle.
Test suite: unit suites unchanged (266/267 pass with the
pre-existing snapshotWeightIntegrity fail). Rehearsal scenarios are
devnet-only (docker compose); same execution model as scenarios
01-08, run via `testing/forks/rehearsal/run-all.sh` or per-scenario
`bun run testing/forks/rehearsal/scenarios/09-fee-distribution.ts`.
Typecheck clean except pre-existing L2PS breakage.
…lder (myc#100, DEM-665 P10b)
Records the end-to-end devnet run of the DEM-665 P10b scenarios:
- Scenario 9 (gasFeeSeparation co-activation) PASS in 169.1s on a
4-node Postgres devnet. All assertions green:
* combined fork crossed at height 5
* osDenomination still activates + sum invariant intact
(regression guard against decimals)
* block-5 hash converged
* gasFeeSeparation fork_state row converged at applied_at_block=5
* burn account 0x000…000 exists, balance 0 on every node
* treasury account exists, balance 0 on every node
* liveness window cleared
- Scenario 10 (burn-spend rejection) PASS in 0.5s as the documented
placeholder it is. Devnet drive blocked on harness signing
infrastructure; coverage at unit level in
tests/blockchain/GCRBalanceRoutines.test.ts (8 cases).
Run command for reproduction:
POSTGRES_HOST_PORT=5532 POSTGRES_USER=demosuser POSTGRES_PASSWORD=demospass \
bun run testing/forks/rehearsal/scenarios/09-fee-distribution.ts
(The PG env vars are needed because testing/devnet/.env sets
POSTGRES_HOST_PORT=5532, while lib/nodeQueries.ts defaults to 5432.)
Final-state cleanup verified — runScenario lifecycle torn down all
demos-devnet-* containers, restored production genesis.
… direct unit tests (myc#101, DEM-665 P10c) Closes the last open follow-up on epic #10. The `applyGasFeeSeparation` helper used to be a module-private function inside validateTransaction.ts; mocking the full confirmTransaction surface (Chain, GCR, forgeToHex, Transaction.confirmTx, every signing helper) to reach it was heavier than the helper itself deserved. Moving it into its own file makes every external dependency jest-mockable in isolation. Files: - src/libs/blockchain/routines/applyGasFeeSeparation.ts (NEW): - Exports `applyGasFeeSeparation(tx) -> Promise<{ ok: true } | { ok: false; message: string }>`. - Same behavior as the inline version: stamps transaction_fee.{network_fee, rpc_fee, additional_fee, rpc_address}, PROD-only balance check, prepends generateFeeDistributionEdits onto tx.content.gcr_edits. - Tighter signature — drops the unused `validityData` parameter the inline version carried. - Re-exports an `ApplyGasFeeSeparationTx` Pick<ITransaction, "content" | "hash"> so callers can pass either the SDK Transaction or the node-side subclass. - src/libs/blockchain/routines/validateTransaction.ts: - Imports applyGasFeeSeparation from the new module. - Inline copy deleted; the fork-gated call site at line 131 now invokes the imported helper with `applyGasFeeSeparation(tx)` (one-arg). - Removed `calculateFeeBreakdown` + `generateFeeDistributionEdits` imports — the helper owns them now. - tests/blockchain/applyGasFeeSeparation.test.ts (NEW): 15 tests organised into four describes: 1. happy path — stamps transaction_fee fields, prepends edits, emits 5 edits for 50/50 network + 100% rpc breakdown. 2. sender address resolution — string passthrough, forgeToHex coercion for non-string, error path when forgeToHex throws. 3. breakdown sanity — rejects NaN, Infinity, fractional, negative totals. Drives them via sharedStateStub.networkFee instead of mocking the calculateFeeBreakdown module (bun test does not auto-isolate module mocks across files; mocking calculateCurrentGas here leaked into tests/governance/calculateCurrentGas.test.ts during the full suite run). 4. PROD balance check — accept on >=, reject on <, error path when GCR.getGCRNativeBalance throws, no balance call in non-PROD. Test suite: 281/282 pass / 715 expect() calls across testing/forks/ + tests/governance/ + tests/blockchain/. The 1 fail is the pre-existing snapshotWeightIntegrity Jest-mock setup issue unrelated to DEM-665. Typecheck clean except pre-existing L2PS breakage from stabilisation merge.
…yc#100, DEM-665 P10b)
Promotes scenario 10 from documented placeholder to a full devnet
drive by adding the missing tx-signing infrastructure to the
rehearsal harness. Scenario 10 now submits a real signed tx with a
malicious remove-from-burn GCREdit and asserts the consensus-critical
invariant (burn balance UNCHANGED on every node) end-to-end.
Result on 4-node Postgres devnet: PASS in 126.4s.
Files:
- testing/forks/rehearsal/lib/signing.ts (NEW):
- generateHarnessKeypair() — fresh ed25519 keypair via
Cryptography.newFromSeed(crypto.randomBytes(32)). Same primitive
ucrypto.generateIdentity uses; derivation is bit-identical to a
production node start. In-memory only, never persisted, devnet
only.
- signHarnessTx(kp, content, blockHeight) — runs the fork-aware
serializeTransactionContent + sha256 + Cryptography.sign chain to
produce {hash, signature} in the shape the validating node
expects via manageExecution({extra: "confirmTx", data: tx}).
- testing/forks/rehearsal/lib/devnetControl.ts:
- stageGenesisWithFundedAccount(fixture, pubkey, balance) — clones
a rehearsal fixture, appends [pubkey, balance] to its balances
array, writes the result to data/genesis.json. Reuses the same
one-time prod-backup stageGenesis() produces.
- testing/forks/rehearsal/scenarios/10-burn-spend-rejection.ts
(rewritten from placeholder):
- Generates fresh harness keypair.
- Injects funded entry into genesis-fork-low-gasFee.json balances.
- Boots 4-node devnet, waits for fork crossing (height >= 6).
- Verifies osDenomination.activated + gasFeeSeparation.applied_at_block
converged.
- Builds a "send" tx whose gcr_edits include the malicious
remove-from-burn entry alongside legitimate sender-remove /
recipient-add.
- Signs with harness keypair via the fork-aware serializer.
- POSTs to node-1 as a confirmTx bundle.
- Sleeps 15s, asserts burn balance == "0" on every node +
fork_state unchanged.
- decimal_planning/REHEARSAL_RESULTS.md: Run 7 section appended with
the full harness procedure + result.
Why "burn balance unchanged" instead of "validator returned specific
message": the rejection mechanism can fire either at confirm-time
(validating node refuses to sign ValidityData) OR at apply-time
(GCRBalanceRoutines.apply returns success:false). Both outcomes
produce the same observable state across nodes. The consensus-
meaningful invariant is balance preservation; the specific message
("Cannot deduct from burn address") is a diagnostic log, not part of
the wire response. Unit suite covers the apply-layer guard message
explicitly (tests/blockchain/GCRBalanceRoutines.test.ts, 8 cases).
Test suite (unit): unchanged, 281/282 pass with the pre-existing
snapshotWeightIntegrity fail. Typecheck clean except pre-existing
L2PS breakage.
Reproduction:
POSTGRES_HOST_PORT=5532 POSTGRES_USER=demosuser POSTGRES_PASSWORD=demospass \\
bun run testing/forks/rehearsal/scenarios/10-burn-spend-rejection.ts
P10b is fully closed.
Contributor
ⓘ You've reached your Qodo monthly free-tier limit. Reviews pause until next month — upgrade your plan to continue now, or link your paid account if you already have one. |
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds DEM-665 "gasFeeSeparation": new fork config/loader, migration to create burn/treasury accounts, transaction ChangesFork configuration, loader, burn address
Migration & activation wiring
Transaction schema, wiring, and persistence
Fee calculation, distribution, and application
Governance safety bounds
Shared runtime state
Tests, rehearsal harness, and operational docs
Sequence Diagram(s)sequenceDiagram
participant Client
participant Node as Node / confirmTransaction
participant Calc as calculateFeeBreakdown
participant Apply as applyGasFeeSeparation
participant Gen as generateFeeDistributionEdits
participant GCR as GCRBalanceApply
participant DB as DB (fork_state / gcr_main)
Client->>Node: submit(tx)
Node->>Node: isForkActive(gasFeeSeparation)?
alt fork active
Node->>Calc: calculateFeeBreakdown(tx)
Calc-->>Node: {network_fee,rpc_fee,additional_fee,total}
Node->>Apply: applyGasFeeSeparation(tx)
Apply->>Gen: generateFeeDistributionEdits(sender, fees, rpc_addr)
Gen-->>Apply: [gcr_edits...]
Apply->>Node: prepend edits to tx.content.gcr_edits
Node->>Node: sign validity (includes edits)
else fork inactive
Node->>Node: legacy composed gas path
end
Node->>DB: insertBlock(tx, edits, ...)
DB->>GCR: apply(edit)*
GCR-->>DB: update balances (persist)
sequenceDiagram
participant Loader as loadForkConfigFromGenesis
participant Validator as validateForkEntry
participant State as SharedState
participant Block as insertBlock
participant Migr as runGasFeeSeparationMigration
participant DB as gcr_main / fork_state
Loader->>Validator: validate gasFeeSeparation payload
Validator-->>Loader: validated config (treasuryAddress checked)
Loader->>State: set forkConfig and prime burn/treasury addresses
Note right of Block: On activation block
Block->>Migr: isGasFeeSeparationMigrationApplied?
Migr-->>Block: not applied
Block->>Migr: runGasFeeSeparationMigration(tx, blockNumber, treasury)
Migr->>DB: ensureZeroAccount(BURN_ADDRESS)
Migr->>DB: ensureZeroAccount(treasuryAddress)
Migr->>DB: upsert fork_state (applied=true, applied_at_block)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Greptile SummaryThis PR implements DEM-665 gas fee separation as a second hard fork (
Confidence Score: 5/5Safe to merge; all eight issues flagged in prior review rounds have been correctly resolved, and the new fee-separation path is thoroughly guarded and tested. Every previously identified concern — the null-rpcAddress fee leak, hardcoded additional_fee, duplicate burn-address constant, allZero initialization guard, silent fee bypass in applyGasFeeSeparation, and both TLSN fee bypasses — is addressed with explicit hard-error guards and 128 new tests across eight suites. The only remaining findings are stylistic: an unused treasuryPct parameter that is only log-visible, two synonymous export names for the same constant, and an inlined regex that duplicates a named constant elsewhere. No files require special attention. The fee-distribution and migration paths are well-covered by unit tests and two devnet rehearsal scenarios. Important Files Changed
Sequence DiagramsequenceDiagram
participant Client
participant confirmTransaction
participant applyGasFeeSeparation
participant calculateFeeBreakdown
participant generateFeeDistributionEdits
participant requireFeeDistribution
participant GCRBalanceRoutines
Client->>confirmTransaction: submit tx (post-fork)
confirmTransaction->>applyGasFeeSeparation: applyGasFeeSeparation(tx)
applyGasFeeSeparation->>calculateFeeBreakdown: calculateFeeBreakdown(tx)
calculateFeeBreakdown-->>applyGasFeeSeparation: "{network_fee, rpc_fee, additional_fee, total}"
applyGasFeeSeparation->>applyGasFeeSeparation: stamp tx.transaction_fee fields + rpc_address
applyGasFeeSeparation->>generateFeeDistributionEdits: "generateFeeDistributionEdits({senderAddress, rpcAddress, ...})"
generateFeeDistributionEdits->>requireFeeDistribution: check feeDistribution primed and non-zero
alt not primed or allZero
requireFeeDistribution-->>generateFeeDistributionEdits: null - return []
generateFeeDistributionEdits-->>applyGasFeeSeparation: []
applyGasFeeSeparation-->>confirmTransaction: "{ok:false, fee distribution not primed}"
else primed
requireFeeDistribution-->>generateFeeDistributionEdits: FeeDistributionRuntime
generateFeeDistributionEdits-->>applyGasFeeSeparation: [remove(sender), add(burn), add(treasury), add(rpcOp)]
applyGasFeeSeparation->>applyGasFeeSeparation: prepend fee edits onto tx.gcr_edits
applyGasFeeSeparation-->>confirmTransaction: "{ok:true}"
confirmTransaction->>GCRBalanceRoutines: apply gcr_edits
GCRBalanceRoutines->>GCRBalanceRoutines: reject remove from burnAddress (post-fork, non-rollback)
end
Reviews (8): Last reviewed commit: "fix(handleError): prefer cause.toString(..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
docs/GAS_FEE_SEPARATION_PLAN.md (1)
83-107: 💤 Low valueConsider adding language identifiers to fenced code blocks.
The two diagram blocks (lines 83-91) are missing language identifiers. While these are flowchart-style diagrams rather than executable code, adding
textormermaididentifiers would satisfy markdown linters and improve rendering.♻️ Proposed fix
-``` +```text TX created → single composedGas calculated → single "pay_gas" Operation → single "remove" GCREdit```diff -``` +```text TX created → 3 fee components calculated separately → rpc_address captured → per-component GCREdits generated: ...</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@docs/GAS_FEE_SEPARATION_PLAN.mdaround lines 83 - 107, The two fenced
diagram blocks starting with "TX created → single composedGas calculated →
single "pay_gas" Operation → single "remove" GCREdit" and "TX created → 3 fee
components calculated separately → rpc_address captured" should include a
language identifier (e.g.,text ormermaid) so markdown linters/renderers
treat them correctly; update those two triple-backtick blocks to start with
text (ormermaid if you prefer flowchart rendering) while leaving the
diagram contents unchanged.</details> </blockquote></details> <details> <summary>testing/forks/rehearsal/lib/devnetControl.ts (1)</summary><blockquote> `242-242`: _💤 Low value_ **Prefer imported `readFileSync` over inline `require("fs")`.** The file already imports `readFileSync` from `fs` at line 14. Using the imported binding is more consistent with the rest of the file and avoids the inline `require` call. <details> <summary>♻️ Proposed fix</summary> ```diff - const raw = require("fs").readFileSync(rehearsalGenesisPath, "utf8") + const raw = readFileSync(rehearsalGenesisPath, "utf8") ``` </details> As per coding guidelines, the static analysis hint suggests preferring `node:fs` over `fs`, but the entire file uses bare `fs` imports, so maintaining consistency within this file is more valuable than mixing import styles. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@testing/forks/rehearsal/lib/devnetControl.ts` at line 242, Replace the inline require("fs").readFileSync call that assigns to raw with the already-imported readFileSync binding: use readFileSync(rehearsalGenesisPath, "utf8") instead of require("fs").readFileSync(...). Update the statement that sets raw (using the rehearsalGenesisPath symbol) so it relies on the module-level imported readFileSync to keep import style consistent with the rest of the file. ``` </details> </blockquote></details> <details> <summary>testing/forks/rehearsal/lib/signing.ts (1)</summary><blockquote> `21-21`: _💤 Low value_ **Consider using `node:crypto` prefix for Node.js core module imports.** This is a style preference for modern Node.js (16+) that makes it explicit when importing from Node.js core vs npm packages. However, the codebase doesn't consistently use the `node:` prefix (e.g., `devnetControl.ts` uses bare `fs` imports), so adopting this would be a broader codebase-wide decision. <details> <summary>♻️ Proposed fix</summary> ```diff -import { randomBytes } from "crypto" +import { randomBytes } from "node:crypto" ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@testing/forks/rehearsal/lib/signing.ts` at line 21, Update the Node core import in signing.ts to use the explicit node: prefix: replace the bare import of randomBytes from "crypto" with the Node-prefixed module so it's clear this is a core module import (look for the top-level import statement that currently reads import { randomBytes } from "crypto"). Ensure the rest of the file (functions using randomBytes) is unchanged and that the new import follows the project's style (you may leave other files alone to avoid a global style change). ``` </details> </blockquote></details> <details> <summary>src/libs/blockchain/transaction.ts (1)</summary><blockquote> `73-77`: _💤 Low value_ **Clarify the "P6" phase reference.** The comment references "P6" which appears to be an internal phase identifier. For long-term maintainability, consider either expanding this acronym inline or linking to the specification document where phases are defined. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/libs/blockchain/transaction.ts` around lines 73 - 77, Update the inline comment above the rpc_address field in transaction.ts to expand or clarify the "P6" phase reference so future readers understand it without internal context; modify the comment that currently reads "confirmTransaction (P6)" to either spell out what P6 stands for (e.g., "Phase 6: validation/confirmation") or add a short parenthetical pointer to the design/spec (e.g., "confirmTransaction (Phase 6 — validating node; see SPEC_SECTION_X)"), leaving the surrounding explanation about pre-fork/null behavior unchanged. ``` </details> </blockquote></details> <details> <summary>src/libs/blockchain/routines/validateTransaction.ts (1)</summary><blockquote> `346-347`: _💤 Low value_ **Clarify the context of this legacy code path.** The comment mentions "internal gas Operation" but the surrounding code (lines 62-76, 218-353) suggests `defineGas` is largely dead code superseded by the GCREdit system. Consider adding a note that this function is deprecated/legacy to help future maintainers understand why `rpc_address: null` is appropriate here. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/libs/blockchain/routines/validateTransaction.ts` around lines 346 - 347, The comment about "internal gas Operation" is unclear—update the legacy note to state that defineGas (and its surrounding logic in validateTransaction.ts) is a deprecated/legacy code path largely superseded by the GCREdit system, and explicitly document why rpc_address is set to null for internal gas operations; reference the defineGas function and the rpc_address: null line so future maintainers understand this is intentional for the legacy internal-gas path rather than a missing RPC routing implementation. ``` </details> </blockquote></details> <details> <summary>testing/forks/rehearsal/scenarios/10-burn-spend-rejection.ts (1)</summary><blockquote> `297-309`: _⚡ Quick win_ **Prefer condition-based waiting over fixed sleep to reduce flakiness.** `await sleep(15_000)` can race on slower environments. Use `waitFor` on the post-submission burn-balance invariant (or tx finality signal) instead of a fixed delay. <details> <summary>Proposed change</summary> ```diff - // Give the network time to settle (any propagation, mempool - // sweep, or apply attempt). - await sleep(15_000) - - // Consensus-critical invariant: burn balance UNCHANGED on every - // node. If the guard is broken, this is where it surfaces. - await assertGcrAccountConvergence( - NODE_IDS, - BURN_ADDRESS, - "0", - "post-submission burn account", - ) + // Wait until the post-submission invariant is observable. + await waitFor( + async () => { + await assertGcrAccountConvergence( + NODE_IDS, + BURN_ADDRESS, + "0", + "post-submission burn account", + ) + return true + }, + { + description: "burn balance remains unchanged after malicious submission", + timeoutMs: 60_000, + intervalMs: 2_000, + }, + ) ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@testing/forks/rehearsal/scenarios/10-burn-spend-rejection.ts` around lines 297 - 309, Replace the fixed await sleep(15_000) with a condition-based wait that polls the post-submission invariant until it succeeds or a timeout elapses: use the existing waitFor utility (or implement a short-poll loop) to repeatedly call assertGcrAccountConvergence(NODE_IDS, BURN_ADDRESS, "0", "post-submission burn account") (catching/asserting failures between polls) at a small interval (e.g., 500–1000ms) and fail the test if the overall timeout (e.g., 30–60s) is exceeded; this ensures we wait for the burn-balance invariant (or a tx finality signal) instead of a fixed 15s sleep and reduces flakiness. ``` </details> </blockquote></details> <details> <summary>src/libs/blockchain/gcr/gcr_routines/feeDistribution.ts (1)</summary><blockquote> `120-171`: _💤 Low value_ **Consider refactoring to an options object to reduce parameter count.** The function has 9 parameters, which exceeds typical maintainability limits (commonly 5-7). While the current implementation is correct and the function is private, refactoring to accept a single options object would improve readability and make future parameter additions easier. <details> <summary>♻️ Example refactor with options object</summary> ```diff +interface TwoRecipientSplitOptions { + componentName: "network_fee" | "additional_fee" + total: number + burnPct: number + treasuryPct: number + burnAddress: string + treasuryAddress: string + senderAddress: string + txHash: string + isRollback: boolean +} + function emitTwoRecipientSplit( - componentName: "network_fee" | "additional_fee", - total: number, - burnPct: number, - treasuryPct: number, - burnAddress: string, - treasuryAddress: string, - senderAddress: string, - txHash: string, - isRollback: boolean, + options: TwoRecipientSplitOptions, ): GCREditBalance[] { + const { + componentName, + total, + burnPct, + treasuryPct, + burnAddress, + treasuryAddress, + senderAddress, + txHash, + isRollback, + } = options if (total <= 0) return [] // ... rest remains the same ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/libs/blockchain/gcr/gcr_routines/feeDistribution.ts` around lines 120 - 171, The function emitTwoRecipientSplit has too many positional parameters; refactor it to accept a single options object (e.g. { componentName, total, burnPct, treasuryPct, burnAddress, treasuryAddress, senderAddress, txHash, isRollback }), destructure those fields at the top of emitTwoRecipientSplit and keep the existing logic and calls to makeBalanceEdit and log.debug unchanged; update all callers of emitTwoRecipientSplit to pass the new options object and adjust any unit tests or usages accordingly so behavior remains identical. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>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@decimal_planning/RUNBOOK_FORK_ACTIVATION.md:
- Line 536: The runbook line for "DEM-665 status" is ambiguous about the SDK
to deploy; replace the existing sentence containing "Source-branch:
claude/gas-fee-separation-aDJK5. SDK companion: 4.0.0-rc.1 (pending publish;
user owns)" with a single unambiguous statement that specifies the exact
required SDK version or an allowed semantic version range (for example "SDK
companion: 4.0.0" or "SDK companion: >=4.0.0 <4.1.0"), and remove the "(pending
publish)" phrase unless it is still true; keep the Source-branch identifier only
if necessary for traceability and ensure the final line reads clearly as the
authoritative SDK requirement for fork activation.In
@src/forks/forkConfig.ts:
- Line 42: The empty interface OsDenominationConfig extends BaseForkConfig and
triggers the no-empty-interface lint rule; replace the interface declaration
with a type alias (e.g., "type OsDenominationConfig = BaseForkConfig;") so
OsDenominationConfig is an alias of BaseForkConfig and the lint error is
resolved, updating the declaration of OsDenominationConfig accordingly while
keeping BaseForkConfig unchanged.In
@src/libs/blockchain/routines/applyGasFeeSeparation.ts:
- Around line 80-85: The catch blocks in applyGasFeeSeparation that build
messages likefailed to resolve sender address: ${msg}use String(e) which
yields "[object Object]" for plain objects; replace that stringification with a
robust helper (or inline) that if e is an Error uses e.name/e.message, otherwise
attempts JSON.stringify(e, null, 2) and falls back to util.inspect(e) or
String(e) on failure, and use that result for both the "failed to resolve sender
address" and the later similar catch (resolvePaymaster) messages so the returned
messagecontains useful serialized details.
Nitpick comments:
In@docs/GAS_FEE_SEPARATION_PLAN.md:
- Around line 83-107: The two fenced diagram blocks starting with "TX created →
single composedGas calculated → single "pay_gas" Operation → single "remove"
GCREdit" and "TX created → 3 fee components calculated separately → rpc_address
captured" should include a language identifier (e.g.,text ormermaid) so
markdown linters/renderers treat them correctly; update those two
triple-backtick blocks to start withtext (ormermaid if you prefer
flowchart rendering) while leaving the diagram contents unchanged.In
@src/libs/blockchain/gcr/gcr_routines/feeDistribution.ts:
- Around line 120-171: The function emitTwoRecipientSplit has too many
positional parameters; refactor it to accept a single options object (e.g. {
componentName, total, burnPct, treasuryPct, burnAddress, treasuryAddress,
senderAddress, txHash, isRollback }), destructure those fields at the top of
emitTwoRecipientSplit and keep the existing logic and calls to makeBalanceEdit
and log.debug unchanged; update all callers of emitTwoRecipientSplit to pass the
new options object and adjust any unit tests or usages accordingly so behavior
remains identical.In
@src/libs/blockchain/routines/validateTransaction.ts:
- Around line 346-347: The comment about "internal gas Operation" is
unclear—update the legacy note to state that defineGas (and its surrounding
logic in validateTransaction.ts) is a deprecated/legacy code path largely
superseded by the GCREdit system, and explicitly document why rpc_address is set
to null for internal gas operations; reference the defineGas function and the
rpc_address: null line so future maintainers understand this is intentional for
the legacy internal-gas path rather than a missing RPC routing implementation.In
@src/libs/blockchain/transaction.ts:
- Around line 73-77: Update the inline comment above the rpc_address field in
transaction.ts to expand or clarify the "P6" phase reference so future readers
understand it without internal context; modify the comment that currently reads
"confirmTransaction (P6)" to either spell out what P6 stands for (e.g., "Phase
6: validation/confirmation") or add a short parenthetical pointer to the
design/spec (e.g., "confirmTransaction (Phase 6 — validating node; see
SPEC_SECTION_X)"), leaving the surrounding explanation about pre-fork/null
behavior unchanged.In
@testing/forks/rehearsal/lib/devnetControl.ts:
- Line 242: Replace the inline require("fs").readFileSync call that assigns to
raw with the already-imported readFileSync binding: use
readFileSync(rehearsalGenesisPath, "utf8") instead of
require("fs").readFileSync(...). Update the statement that sets raw (using the
rehearsalGenesisPath symbol) so it relies on the module-level imported
readFileSync to keep import style consistent with the rest of the file.In
@testing/forks/rehearsal/lib/signing.ts:
- Line 21: Update the Node core import in signing.ts to use the explicit node:
prefix: replace the bare import of randomBytes from "crypto" with the
Node-prefixed module so it's clear this is a core module import (look for the
top-level import statement that currently reads import { randomBytes } from
"crypto"). Ensure the rest of the file (functions using randomBytes) is
unchanged and that the new import follows the project's style (you may leave
other files alone to avoid a global style change).In
@testing/forks/rehearsal/scenarios/10-burn-spend-rejection.ts:
- Around line 297-309: Replace the fixed await sleep(15_000) with a
condition-based wait that polls the post-submission invariant until it succeeds
or a timeout elapses: use the existing waitFor utility (or implement a
short-poll loop) to repeatedly call assertGcrAccountConvergence(NODE_IDS,
BURN_ADDRESS, "0", "post-submission burn account") (catching/asserting failures
between polls) at a small interval (e.g., 500–1000ms) and fail the test if the
overall timeout (e.g., 30–60s) is exceeded; this ensures we wait for the
burn-balance invariant (or a tx finality signal) instead of a fixed 15s sleep
and reduces flakiness.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Organization UI **Review profile**: CHILL **Plan**: Pro **Run ID**: `3c906360-ae9d-4a3c-b0b8-0b569448d559` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 82b58418bb84d4c56e6afedb2c591bd484c92d4f and bcaec89601bcf14475eaaa6e6172343ef3c364c3. </details> <details> <summary>📒 Files selected for processing (52)</summary> * `decimal_planning/REHEARSAL_RESULTS.md` * `decimal_planning/RUNBOOK_FORK_ACTIVATION.md` * `docs/GAS_FEE_SEPARATION_PLAN.md` * `package.json` * `src/features/InstantMessagingProtocol/signalingServer/signalingServer.ts` * `src/features/networkUpgrade/constants.ts` * `src/features/networkUpgrade/safetyBounds.ts` * `src/forks/forkConfig.ts` * `src/forks/index.ts` * `src/forks/loadForkConfig.ts` * `src/forks/migrations/gasFeeSeparation.ts` * `src/forks/serializerGate.ts` * `src/libs/blockchain/chainBlocks.ts` * `src/libs/blockchain/chainGenesis.ts` * `src/libs/blockchain/gcr/gcr_routines/GCRBalanceRoutines.ts` * `src/libs/blockchain/gcr/gcr_routines/feeDistribution.ts` * `src/libs/blockchain/gcr/gcr_routines/handleNativeOperations.ts` * `src/libs/blockchain/routines/applyGasFeeSeparation.ts` * `src/libs/blockchain/routines/calculateCurrentGas.ts` * `src/libs/blockchain/routines/loadNetworkParameters.ts` * `src/libs/blockchain/routines/validateTransaction.ts` * `src/libs/blockchain/transaction.ts` * `src/libs/l2ps/L2PSBatchAggregator.ts` * `src/libs/utils/demostdlib/deriveMempoolOperation.ts` * `src/migrations/AddRpcAddressToTransactions.ts` * `src/model/entities/Transactions.ts` * `src/utilities/sharedState.ts` * `testing/forks/amountCanonical.test.ts` * `testing/forks/disableForkMachineryFlag.test.ts` * `testing/forks/forkBoundary.test.ts` * `testing/forks/forkGates.test.ts` * `testing/forks/getNetworkInfo.test.ts` * `testing/forks/integration.test.ts` * `testing/forks/loadForkConfig.test.ts` * `testing/forks/migrations/gasFeeSeparation.test.ts` * `testing/forks/migrations/genesisFailLoud.test.ts` * `testing/forks/postForkSerializer.test.ts` * `testing/forks/rehearsal/genesis/genesis-fork-low-gasFee.json` * `testing/forks/rehearsal/lib/assertions.ts` * `testing/forks/rehearsal/lib/devnetControl.ts` * `testing/forks/rehearsal/lib/nodeQueries.ts` * `testing/forks/rehearsal/lib/signing.ts` * `testing/forks/rehearsal/run-all.sh` * `testing/forks/rehearsal/scenarios/09-fee-distribution.ts` * `testing/forks/rehearsal/scenarios/10-burn-spend-rejection.ts` * `testing/forks/serializerGate.test.ts` * `tests/blockchain/GCRBalanceRoutines.test.ts` * `tests/blockchain/applyGasFeeSeparation.test.ts` * `tests/blockchain/feeDistribution.test.ts` * `tests/blockchain/handleNativeOperations.test.ts` * `tests/governance/calculateCurrentGas.test.ts` * `tests/governance/safetyBounds.test.ts` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
…all-10 green (DEM-665)
Two pre-existing harness bugs surfaced during the full run-all.sh
acceptance gate for the DEM-665 branch. Fixed both; rehearsal cycle
now ends 10/10 PASS in 1929s.
Bug 1 — run-all.sh empty-array expansion under macOS bash 3.2:
set -u + "${EMPTY_ARRAY[@]}" throws "unbound variable" on bash
3.2. The runner crashed on the first scenario before bun ever
fired. Switched to the portable
${EXTRA_ARGS[@]+"${EXTRA_ARGS[@]}"} parameter expansion which only
emits words when the array is non-empty.
Bug 2 — rpcNodeCall wire shape (lib/nodeQueries.ts):
The harness flattened extraParams alongside message
(params: [{ message, ...extraParams }]). manageNodeCall unpacks
content.data and forwards it to every handler; handlers expect
data.address (or similar). The flattened shape left
data === undefined, every parametric nodeCall returned
"Error in nodeCall: TypeError: undefined is not an object".
Pre-myc#86 the bug was invisible because cross-node assertions
compared Set.size === 1 of all-null returns and passed trivially.
myc#86 strictened the null-check and scenario 06 started failing
on every run.
Fix: wrap extras under data:
params: [{
message,
data: Object.keys(extraParams).length > 0
? extraParams
: {},
}]
Parameter-free RPCs (getLastBlockNumber, getNetworkInfo, ...) are
unaffected because their handlers never read data.
Run 8 results appended to decimal_planning/REHEARSAL_RESULTS.md:
PASS 04-genesis-hash-invariance 93s
PASS 01-all-cross-fork 168s
PASS 07-sum-invariant-audit 161s
PASS 08-idempotent-restart 120s
PASS 05-cap-policy-fires-loud 241s
PASS 06-mid-flight-tx 248s
PASS 02-validator-desync-recovery 169s
PASS 03-fresh-node-post-fork 434s
PASS 09-fee-distribution 166s
PASS 10-burn-spend-rejection 129s
Total: 1929s wall-clock.
Side note for ops: a long rehearsal cycle can corrupt the Docker
buildkit snapshot graph
("parent snapshot ... does not exist" extraction failures).
`docker system prune -a -f --volumes` rebuilds it cleanly. Document
as a between-cycle housekeeping step.
DEM-665 implementation is gated clean against the full rehearsal
cycle. Branch ready for review.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
decimal_planning/REHEARSAL_RESULTS.md (1)
695-706: ⚡ Quick winAdd language specifier to fenced code block.
The fenced code block starting at line 695 should specify a language for proper syntax highlighting and markdown linting compliance.
📝 Proposed fix
-``` +```text PASS 04-genesis-hash-invariance 93s PASS 01-all-cross-fork 168s PASS 07-sum-invariant-audit 161s🤖 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 `@decimal_planning/REHEARSAL_RESULTS.md` around lines 695 - 706, The fenced code block that lists the PASS lines (e.g., "PASS 04-genesis-hash-invariance 93s" etc.) should include a language specifier for markdown linting and highlighting; update the opening fence from ``` to ```text (or another appropriate language) so the block becomes ```text followed by the existing lines and the closing ```.
🤖 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.
Nitpick comments:
In `@decimal_planning/REHEARSAL_RESULTS.md`:
- Around line 695-706: The fenced code block that lists the PASS lines (e.g.,
"PASS 04-genesis-hash-invariance 93s" etc.) should include a language specifier
for markdown linting and highlighting; update the opening fence from ``` to
```text (or another appropriate language) so the block becomes ```text followed
by the existing lines and the closing ```.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 63f6ea61-fe4a-4830-ab0e-864fd43976ab
📒 Files selected for processing (3)
decimal_planning/REHEARSAL_RESULTS.mdtesting/forks/rehearsal/lib/nodeQueries.tstesting/forks/rehearsal/run-all.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- testing/forks/rehearsal/run-all.sh
added 2 commits
May 12, 2026 17:19
Greptile review of PR #817 surfaced 3 P1 + 2 P2 findings. All addressed: P1 — feeDistribution.ts rpc_fee silent leak (line 250) Previous code dropped the entire rpc_fee block when rpcAddress was null — no remove from sender, no add to anyone, sender silently kept the tokens. Inconsistent with generateSpecialOpsFeeEdits which folds rpc share into treasury. Fix: sender's remove ALWAYS fires; rpc share folds into treasury when no rpc operator is identified. Updated the relevant unit test to assert sender-removed + treasury-credited + sum invariant. P1 — calculateCurrentGas.ts additional_fee ignored governance (line 81) additional_fee was hardcoded to 0 while the additionalFee key was wired into PHASE_1_GOVERNABLE_KEYS + safetyBounds + the SDK NetworkParameters shape. A passing governance proposal raising it would have been stored but invisible to the collection path — silent fee-leak surface. Fix: added additionalFee scalar to SharedState (default 0), loadNetworkParameters mirrors NetworkParameters.additionalFee onto the scalar, calculateFeeBreakdown reads from shared state. New unit test asserts governance-driven additionalFee actually surfaces in the breakdown. P1 — loadForkConfig.ts BURN_ADDRESS duplicate constant (line 46) loadForkConfig.ts and migrations/gasFeeSeparation.ts both declared their own "0x" + "0".repeat(64) literal, with a unit test guarding equality. If the test were skipped (or the pre-existing failure masked it) the two could drift — migration creates the burn account at one literal while the runtime guard reads from another, allowing burned coins to be re-circulated. Fix: new leaf module src/forks/burnAddress.ts owns the single source of truth. Both modules import from it and re-export under their previous names so call sites stay stable. Invariant is now compile-time. P2 — feeDistribution.ts zero-percent init window (line 258) loadForkConfigFromGenesis primes feeDistribution with percentages = 0 to keep the structure non-null before loadNetworkParameters folds governance values. A post-fork tx processed in that window would route 100% of fees to treasury invisibly because all burn/rpc shares would be 0. Fix: requireFeeDistribution now refuses to emit edits when all seven percentage fields are zero, with a clear log line. The caller (applyGasFeeSeparation) surfaces the rejection through the standard ValidityData failure path. P2 — gasFeeSeparation.ts sqlite ON CONFLICT portability (line 326) ON CONFLICT (col) DO UPDATE SET … EXCLUDED.* requires sqlite ≥ 3.24.0. Documented as a portability note in-code; verified Bun's bundled sqlite is 3.44.2 (well above floor). The osDenomination migration already uses the same syntax, so the floor is implicit in the rehearsal stack; the doc is for future image downgrades. Test suite: 282/283 pass (1 pre-existing unrelated fail). Unit suites for fee-distribution, calculateFeeBreakdown, and applyGasFeeSeparation all green with the updated semantics. Typecheck clean except pre-existing L2PS breakage. Files: - src/forks/burnAddress.ts (NEW): single source of truth. - src/forks/loadForkConfig.ts: re-export from leaf. - src/forks/migrations/gasFeeSeparation.ts: re-export + sqlite note. - src/libs/blockchain/gcr/gcr_routines/feeDistribution.ts: rpc-fold-to-treasury + zero-percent guard. - src/libs/blockchain/routines/calculateCurrentGas.ts: additional_fee from sharedState. - src/libs/blockchain/routines/loadNetworkParameters.ts: mirror additionalFee onto sharedState. - src/utilities/sharedState.ts: additionalFee field. - tests/blockchain/feeDistribution.test.ts: null-rpc test updated. - tests/blockchain/applyGasFeeSeparation.test.ts: stub adds additionalFee. - tests/governance/calculateCurrentGas.test.ts: stub adds additionalFee + new governance-mutability test.
Three additional findings from CodeRabbit's review pass, all addressed: #1 — RUNBOOK_FORK_ACTIVATION.md:536 SDK version ambiguity Final line said "SDK companion: 4.0.0-rc.1 (pending publish; user owns)". The PR has since shipped the 3.1.0 → 4.0.0 pin bump (commit b8e9bff) so the rc-1 / pending-publish wording is now stale and could mislead validators about which version to deploy. Fix: changed to the unambiguous "**`@kynesyslabs/demosdk@4.0.0`** (required at and after fork activation; pinned in `node/package.json`)" form. #2 — forkConfig.ts:42 empty interface `interface OsDenominationConfig extends BaseForkConfig {}` trips `@typescript-eslint/no-empty-object-type`. The type carries no additional members; an empty interface adds no information over a plain alias. Fix: switched to `export type OsDenominationConfig = BaseForkConfig` with a comment explaining the why (CodeRabbit feedback + the consumer-narrows-via-ForkName invariant). Structurally identical; zero runtime impact. #3 — applyGasFeeSeparation.ts:81 + 120 String(e) on non-Error Both catch blocks did `e instanceof Error ? e.message : String(e)`. `String(plainObject)` collapses to "[object Object]" — the structured shape vanishes when something other than an Error gets thrown (e.g. a raw RPC body). SonarCloud also flagged this. Fix: new `stringifyNonError(e)` helper that JSON.stringify's the value, falling back to `String(e)` if serialisation itself throws (cyclic graph, BigInt without rawJSON). The diagnostic path never re-throws. Test suite: 282/283 pass (1 pre-existing unrelated fail). Typecheck clean except pre-existing L2PS breakage.
added 2 commits
May 12, 2026 17:42
Single coherent operator-facing runbook for the combined fork
(osDenomination + gasFeeSeparation), one folder per design spec.
14 stale planning docs removed.
New layout:
forking/
RUNBOOK_FORK_ACTIVATION.md ← combined runbook (new home, was
decimal_planning/RUNBOOK_…)
REHEARSAL_RESULTS.md ← 8 rehearsal-run audit trail
decimal_planning/
SPEC.md ← DEM→OS denomination design rationale
gas_separation/
PLAN.md ← as-shipped DEM-665 plan
(was docs/GAS_FEE_SEPARATION_PLAN.md)
The runbook is rewritten as a single 10-section flow rather than the
prior "decimals body + DEM-665 appendix" structure. Treats the
combined fork as one event:
§0 TL;DR
§1 one-screen architecture
§2 pre-flight checklist (covers both forks)
§3 height selection + seal genesis
§4 fork-day timeline with the combined log sequence
(osDenomination FIRST then gasFeeSeparation, ordered in
chainBlocks)
§5 "what right looks like" reference values
§6 recovery procedures
§7 post-fork operational notes
§8 governance — mutable distribution percentages
§9 don't-do list (combined)
§10 references — file:line map for code + design docs
Deleted (superseded):
decimal_planning/IDEA.md — initial brainstorm
decimal_planning/SPEC_P4.md — SDK 3.0.0-rc.1 phase
decimal_planning/forking_feasibility.md — pre-impl feasibility
decimal_planning/NEXT_STEPS.md — phase planning
decimal_planning/PAUSED.md — wait-state note
decimal_planning/PR_REVIEW_DISMISSED.md — per-PR notes
decimal_planning/PR_REVIEW_DISMISSED_812.md
decimal_planning/audit_node.md — pre-impl audits
decimal_planning/audit_node_post_staking.md
decimal_planning/audit_sdk.md
decimal_planning/surface_scan.md — early code-scan
decimal_planning/LOG.md — session logbook (git history)
decimal_planning/DEVNET_READINESS.md — one-shot pre-rehearsal audit
decimal_planning/REHEARSAL_PLAN.md — rehearsal design (harness
IS the plan now)
decimal_planning/ — empty directory removed
In-tree references updated to point at the new paths:
src/config/{types,defaults}.ts
src/forks/amountCanonical.ts
testing/forks/{forkGates.test.ts,preflight.ts}
testing/forks/rehearsal/README.md
forking/RUNBOOK_FORK_ACTIVATION.md (self)
forking/REHEARSAL_RESULTS.md
forking/gas_separation/PLAN.md
Test suite: 282/283 pass (1 pre-existing unrelated fail). Typecheck
clean except pre-existing L2PS breakage.
…dits emitted (Greptile P1 round 3)
Greptile flagged a silent fee-bypass path on the latest review pass:
generateFeeDistributionEdits returns [] when
requireFeeDistribution() returns null — either feeDistribution is
itself null OR every percentage is still 0 (the transient init
window between loadForkConfigFromGenesis and loadNetworkParameters).
applyGasFeeSeparation was prepending the empty array onto
gcr_edits and returning { ok: true } in that case. The sender's
balance was checked against breakdown.total but no deduction
edits were emitted: the total fee was silently forgiven while
the transaction was accepted.
The justification comment in feeDistribution.ts claimed
"the caller surfaces the rejection through applyGasFeeSeparation's
failure path" — the actual code path was silent success.
Fix: applyGasFeeSeparation now refuses the tx whenever
breakdown.total > 0 AND generateFeeDistributionEdits returned 0
edits. Returns { ok: false, message: "fee distribution not primed
— refusing to accept post-fork tx without fee collection" }. The
ValidityData failure path signs that message into the response,
matching the original intent.
Zero-total txs still pass — the no-edits return is correct in that
case.
Tests: 3 new cases in tests/blockchain/applyGasFeeSeparation.test.ts:
- rejects post-fork tx when feeDistribution is null
- rejects post-fork tx when all percentages are 0 (init window)
- accepts zero-total tx even when feeDistribution returns no edits
beforeEach/afterEach were widened to restore feeDistribution to the
SPEC defaults between cases so a sibling case can mutate it without
leaking state to the PROD-balance-check block.
Test suite: 282/283 pass (1 pre-existing snapshotWeightIntegrity
fail unrelated). 18/18 in tests/blockchain/applyGasFeeSeparation.test.ts.
[UNKNOWN_ERROR] log lines previously dropped the underlying cause, leaving operators with a one-line "Connection terminated unexpectedly" or an empty AggregateError message and no stack — boot failures looked like the node had no reason to fall over. The cause is forwarded into AppError by normalizeError but logError never read it. formatCauseChain walks `error.cause` (Node 16+ chained cause) AND `error.errors[]` (AggregateError siblings — happy-eyeballs IPv4/IPv6 connect failures and parallel-promise rejections live here). Depth and sibling count are bounded so a cyclical or pathological chain can't flood the logger. This surfaced the underlying pg "Connection terminated unexpectedly" during a stale-data-dir boot — exactly the kind of diagnostic the operator should see without attaching a debugger.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/errors/handleError.ts (1)
223-264: 💤 Low valueReduce cognitive complexity by extracting AggregateError handling.
SonarCloud flags this function at cognitive complexity 16 (limit 15). The
Array.isArray(siblings)branch with its nested loop and overflow message is the bulk of the score — pulling it into a small helper drops the complexity below the threshold without changing behavior.♻️ Suggested refactor
+function formatAggregateSiblings( + siblings: unknown[], + indent: string, + depth: number, +): string { + const shown = siblings.slice(0, 5) + let out = "" + for (let i = 0; i < shown.length; i++) { + out += `\n${indent}[error ${i + 1}/${siblings.length}]` + out += formatCauseChain(shown[i], depth + 1) + } + if (siblings.length > shown.length) { + out += `\n${indent}... ${siblings.length - shown.length} more` + } + return out +} + function formatCauseChain(cause: unknown, depth = 0): string { ... const siblings = (cause as { errors?: unknown }).errors if (Array.isArray(siblings) && siblings.length > 0) { - const shown = siblings.slice(0, 5) - for (let i = 0; i < shown.length; i++) { - out += `\n${indent}[error ${i + 1}/${siblings.length}]` - out += formatCauseChain(shown[i], depth + 1) - } - if (siblings.length > shown.length) { - out += `\n${indent}... ${siblings.length - shown.length} more` - } + out += formatAggregateSiblings(siblings, indent, depth) }🤖 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 `@src/errors/handleError.ts` around lines 223 - 264, Extract the AggregateError siblings handling from formatCauseChain into a small helper (e.g., formatAggregateSiblings(siblings: unknown[], depth: number): string) and replace the Array.isArray(siblings) block with a call to that helper; the helper should take the shown = siblings.slice(0,5), loop over shown appending "\n${indent}[error ${i+1}/${siblings.length}]" and recursively call formatCauseChain(shown[i], depth+1) for each, add the overflow line "\n${indent}... ${siblings.length - shown.length} more" when applicable, and preserve the existing indent computation and return string composition so formatCauseChain's behavior, recursion into nested cause, and output formatting remain unchanged.src/libs/blockchain/gcr/gcr_routines/feeDistribution.ts (1)
146-197: ⚡ Quick winConsider consolidating parameters into a structured object.
emitTwoRecipientSplitaccepts 9 parameters, exceeding typical maintainability guidelines (max 7). While the function works correctly, an object parameter would improve readability and make future changes easier.♻️ Refactor to use a parameter object
+interface TwoRecipientSplitInput { + componentName: "network_fee" | "additional_fee" + total: number + burnPct: number + treasuryPct: number + burnAddress: string + treasuryAddress: string + senderAddress: string + txHash: string + isRollback: boolean +} + function emitTwoRecipientSplit( - componentName: "network_fee" | "additional_fee", - total: number, - burnPct: number, - treasuryPct: number, - burnAddress: string, - treasuryAddress: string, - senderAddress: string, - txHash: string, - isRollback: boolean, + input: TwoRecipientSplitInput, ): GCREditBalance[] { + const { componentName, total, burnPct, treasuryPct, burnAddress, treasuryAddress, senderAddress, txHash, isRollback } = input if (total <= 0) return [] // ... rest unchangedThen update call sites to pass an 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 `@src/libs/blockchain/gcr/gcr_routines/feeDistribution.ts` around lines 146 - 197, The function emitTwoRecipientSplit has too many positional params—refactor it to accept a single parameter object (e.g., EmitTwoRecipientSplitArgs) containing componentName, total, burnPct, treasuryPct, burnAddress, treasuryAddress, senderAddress, txHash, and isRollback; update the function signature and internal references to use the object properties and leave logic unchanged (references: emitTwoRecipientSplit, makeBalanceEdit, GCREditBalance), then update all call sites to pass the new object shape instead of nine positional arguments.src/utilities/sharedState.ts (1)
266-266: 💤 Low valueRemove inferrable type annotation.
The type
numberis trivially inferred from the literal0, making the explicit annotation redundant.♻️ Optional cleanup
- additionalFee: number = 0 + additionalFee = 0As per coding guidelines, this project uses TypeScript as the primary language, and removing redundant type annotations follows TypeScript best practices.
🤖 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 `@src/utilities/sharedState.ts` at line 266, The property declaration additionalFee: number = 0 has a redundant explicit type since TypeScript can infer number from the literal; update the declaration for the symbol additionalFee to remove the explicit ": number" annotation so it reads with the initializer only (additionalFee = 0) while preserving semantics and any surrounding comments or visibility modifiers.
🤖 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 `@forking/REHEARSAL_RESULTS.md`:
- Around line 695-706: Add a language identifier to the fenced code block that
contains the PASS lines (the block starting with "PASS
04-genesis-hash-invariance ...") so markdownlint MD040 is satisfied; update the
opening fence from ``` to ```text (or another appropriate language tag) in the
REHEARSAL_RESULTS.md fenced code block so the block is explicitly marked as
plain text.
In `@forking/RUNBOOK_FORK_ACTIVATION.md`:
- Around line 11-19: Several fenced code blocks in the
RUNBOOK_FORK_ACTIVATION.md diff are missing language identifiers (tripping
MD040); update each unlabeled triple-backtick block to include a language tag
(e.g., "text"). Specifically, add the tag to the ASCII box block starting with
"┌─────────────────", the log examples like "[FORKS] Loaded fork
\"osDenomination\"...", "[FORKS] DEMOS_DISABLE_FORK_MACHINERY...", the fork
table snippets containing "fork_name = osDenomination" and "fork_name =
gasFeeSeparation", the pubkey/balance listing, and the warning line "[WARNING]
[forks][osDenomination] CAP applied..." (and the other occurrences mentioned:
225-228, 234-236, 317-325, 329-334, 338-342, 346-350, 354-356) by changing ```
to ```text so markdownlint no longer flags MD040.
In `@src/errors/handleError.ts`:
- Around line 229-235: The cause output duplicates the error header because you
take the full stack (which already starts with "Name: message") and then prepend
`${cause.name}: ${cause.message}` again; fix this in the if (cause instanceof
Error) block in handleError.ts by removing the stack's header line before
slicing — i.e. split the stack and use .slice(1, 6) (or otherwise drop the first
element) when building `frames`, then keep the explicit ` | cause:
${cause.name}: ${cause.message}\n${frames}` so the name/message appears only
once; update the `frames` construction that currently does
`.split("\n").slice(0, 6)` accordingly.
---
Nitpick comments:
In `@src/errors/handleError.ts`:
- Around line 223-264: Extract the AggregateError siblings handling from
formatCauseChain into a small helper (e.g., formatAggregateSiblings(siblings:
unknown[], depth: number): string) and replace the Array.isArray(siblings) block
with a call to that helper; the helper should take the shown =
siblings.slice(0,5), loop over shown appending "\n${indent}[error
${i+1}/${siblings.length}]" and recursively call formatCauseChain(shown[i],
depth+1) for each, add the overflow line "\n${indent}... ${siblings.length -
shown.length} more" when applicable, and preserve the existing indent
computation and return string composition so formatCauseChain's behavior,
recursion into nested cause, and output formatting remain unchanged.
In `@src/libs/blockchain/gcr/gcr_routines/feeDistribution.ts`:
- Around line 146-197: The function emitTwoRecipientSplit has too many
positional params—refactor it to accept a single parameter object (e.g.,
EmitTwoRecipientSplitArgs) containing componentName, total, burnPct,
treasuryPct, burnAddress, treasuryAddress, senderAddress, txHash, and
isRollback; update the function signature and internal references to use the
object properties and leave logic unchanged (references: emitTwoRecipientSplit,
makeBalanceEdit, GCREditBalance), then update all call sites to pass the new
object shape instead of nine positional arguments.
In `@src/utilities/sharedState.ts`:
- Line 266: The property declaration additionalFee: number = 0 has a redundant
explicit type since TypeScript can infer number from the literal; update the
declaration for the symbol additionalFee to remove the explicit ": number"
annotation so it reads with the initializer only (additionalFee = 0) while
preserving semantics and any surrounding comments or visibility modifiers.
🪄 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: aefc0127-263c-412a-82b2-69e542fa58f4
📒 Files selected for processing (37)
.bankdecimal_planning/DEVNET_READINESS.mddecimal_planning/IDEA.mddecimal_planning/LOG.mddecimal_planning/NEXT_STEPS.mddecimal_planning/PAUSED.mddecimal_planning/REHEARSAL_PLAN.mddecimal_planning/RUNBOOK_FORK_ACTIVATION.mddecimal_planning/SPEC_P4.mddecimal_planning/audit_node.mddecimal_planning/audit_node_post_staking.mddecimal_planning/audit_sdk.mddecimal_planning/forking_feasibility.mddecimal_planning/surface_scan.mdforking/REHEARSAL_RESULTS.mdforking/RUNBOOK_FORK_ACTIVATION.mdforking/decimal_planning/SPEC.mdforking/gas_separation/PLAN.mdsrc/config/defaults.tssrc/config/types.tssrc/errors/handleError.tssrc/forks/amountCanonical.tssrc/forks/burnAddress.tssrc/forks/forkConfig.tssrc/forks/loadForkConfig.tssrc/forks/migrations/gasFeeSeparation.tssrc/libs/blockchain/gcr/gcr_routines/feeDistribution.tssrc/libs/blockchain/routines/applyGasFeeSeparation.tssrc/libs/blockchain/routines/calculateCurrentGas.tssrc/libs/blockchain/routines/loadNetworkParameters.tssrc/utilities/sharedState.tstesting/forks/forkGates.test.tstesting/forks/preflight.tstesting/forks/rehearsal/README.mdtests/blockchain/applyGasFeeSeparation.test.tstests/blockchain/feeDistribution.test.tstests/governance/calculateCurrentGas.test.ts
💤 Files with no reviewable changes (13)
- decimal_planning/surface_scan.md
- decimal_planning/LOG.md
- decimal_planning/audit_node.md
- decimal_planning/PAUSED.md
- decimal_planning/SPEC_P4.md
- decimal_planning/forking_feasibility.md
- decimal_planning/REHEARSAL_PLAN.md
- decimal_planning/IDEA.md
- decimal_planning/audit_node_post_staking.md
- decimal_planning/DEVNET_READINESS.md
- decimal_planning/audit_sdk.md
- decimal_planning/NEXT_STEPS.md
- decimal_planning/RUNBOOK_FORK_ACTIVATION.md
✅ Files skipped from review due to trivial changes (6)
- .bank
- testing/forks/preflight.ts
- src/config/defaults.ts
- testing/forks/rehearsal/README.md
- src/forks/amountCanonical.ts
- src/config/types.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/governance/calculateCurrentGas.test.ts
- testing/forks/forkGates.test.ts
- src/libs/blockchain/routines/loadNetworkParameters.ts
- src/forks/forkConfig.ts
…e tags Three review-comment fixes bundled. Each maps to a Mycelium task under epic E#11 (PR #817 review fixes). myc#102 — [G-1 + G-2] handleNativeOperations.ts (P1, critical) Mirror the applyGasFeeSeparation silent-fee-bypass guard onto the two post-fork TLSN branches (tlsn_request, tlsn_store). When generateSpecialOpsFeeEdits returns [] but the demanded fee is > 0 (requireFeeDistribution() null because feeDistribution was never primed OR every percentage is 0), the previous code spread the empty array onto `edits` and the TLSN operation proceeded without removing any tokens from the sender. Now both branches throw with a clear message so the tx is rejected at validation time. Adds one test per branch asserting the throw. myc#104 — [CR-3] handleError.ts (CodeRabbit minor) formatCauseChain was printing `${cause.name}: ${cause.message}` explicitly AND keeping that same line from Error.stack[0], producing duplicate header lines in the boot log. Skip the stack's first line when it begins with `${name}:`. Extracted the AggregateError sibling walk into formatAggregateSiblings to drop SonarCloud cognitive complexity from 16 to below the 15 threshold flagged in the same review. myc#103 — [CR-1 + CR-2] forking/*.md (CodeRabbit minor) Tagged every bare opening fence in REHEARSAL_RESULTS.md and RUNBOOK_FORK_ACTIVATION.md with `text` so markdownlint MD040 is satisfied. ASCII boxes, log snippets, and table dumps all read as plain text; nothing in either file is a real syntax-highlighted language. Tests: 287 pass / 1 pre-existing fail unrelated (governance snapshot test fails on baseline too — module-level Datasource.getInstance is undefined; tracked separately).
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/errors/handleError.ts (1)
289-293: 💤 Low valueConsider improving fallback for non-serializable causes.
The
String(cause)fallback (line 292) will produce"[object Object]"for plain objects when JSON.stringify fails, reducing diagnostic value. While this is acceptable as a last-resort safeguard, you could check for a customtoStringmethod first:try { return ` | cause: ${JSON.stringify(cause)}` } catch { if (typeof cause === "object" && cause !== null && "toString" in cause) { try { return ` | cause: ${cause.toString()}` } catch {} } return ` | cause: ${String(cause)}` }However, given this is already a fallback path for edge cases (circular references or exotic thrown values), the current implementation is reasonable.
🤖 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 `@src/errors/handleError.ts` around lines 289 - 293, In src/errors/handleError.ts, improve the fallback when JSON.stringify(cause) throws by, inside the catch block for the existing try that returns ` | cause: ${JSON.stringify(cause)}`, checking whether cause is a non-null object with a custom toString (i.e., "toString" in cause) and, if so, call that in a nested try and return ` | cause: ${cause.toString()}`; if that fails or no custom toString exists, fall back to returning ` | cause: ${String(cause)}`—this preserves the original behavior but gives more useful diagnostics for plain objects before resorting to String(cause).
🤖 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.
Nitpick comments:
In `@src/errors/handleError.ts`:
- Around line 289-293: In src/errors/handleError.ts, improve the fallback when
JSON.stringify(cause) throws by, inside the catch block for the existing try
that returns ` | cause: ${JSON.stringify(cause)}`, checking whether cause is a
non-null object with a custom toString (i.e., "toString" in cause) and, if so,
call that in a nested try and return ` | cause: ${cause.toString()}`; if that
fails or no custom toString exists, fall back to returning ` | cause:
${String(cause)}`—this preserves the original behavior but gives more useful
diagnostics for plain objects before resorting to String(cause).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0cf8f109-0777-4ba1-83cf-6ace7c5b5de9
📒 Files selected for processing (5)
forking/REHEARSAL_RESULTS.mdforking/RUNBOOK_FORK_ACTIVATION.mdsrc/errors/handleError.tssrc/libs/blockchain/gcr/gcr_routines/handleNativeOperations.tstests/blockchain/handleNativeOperations.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/libs/blockchain/gcr/gcr_routines/handleNativeOperations.ts
- tests/blockchain/handleNativeOperations.test.ts
…lizable causes CodeRabbit nit on PR #817: when JSON.stringify(cause) fails (cycles, exotic thrown values), the catch block dropped to `String(cause)` which collapses plain objects to "[object Object]". Try the cause's own toString first — most thrown objects either carry a useful one or fall through to String() anyway. Pure diagnostic quality improvement; no behavior change on the happy path.
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements DEM-665 — splits the single lump-sum gas deduction into three fee components (
network_fee,rpc_fee,additional_fee) with distinct distribution rules across burn / treasury / rpc-operator recipients. Bundled into a second hard fork namedgasFeeSeparationthat sharesactivationHeightwithosDenomination— one coordinated chain wipe, two state migrations.Linear: DEM-665. Plan doc:
docs/GAS_FEE_SEPARATION_PLAN.md(as-shipped header up top calls out deviations from the original spec). Runbook:decimal_planning/RUNBOOK_FORK_ACTIVATION.md§9.Architecture decisions
ForkConfigis now a discriminated union;OsDenominationConfigunchanged + newGasFeeSeparationConfigcarryingtreasuryAddressonly0x+ 64 zeros insrc/forks/migrations/gasFeeSeparation.ts. Never rotates, not in genesisNetworkParameterswith tighter bounds: ±10% per proposal (vs default ±50%), sum-100 cross-key invariantadditionalFeeNetworkParametersfield. Default 0. GovernableburnFeerpc_address(tx field)confirmTransactionpackage.jsonbumped)Genesis-hash caveat
block.content.extraIS hashed (verified atchainGenesis.ts:60-73). Any change togenesisData.forkschanges the genesis hash and invalidates every existingchain.db. Coordinated chain wipe is required — already planned for decimals;gasFeeSeparationpiggybacks at no extra operator cost.Code map
src/forks/forkConfig.tssrc/forks/loadForkConfig.tssrc/forks/migrations/gasFeeSeparation.tssrc/libs/blockchain/chainBlocks.tssrc/libs/blockchain/routines/calculateCurrentGas.ts(calculateFeeBreakdown)src/libs/blockchain/gcr/gcr_routines/feeDistribution.tssrc/libs/blockchain/routines/applyGasFeeSeparation.ts(extracted module)src/libs/blockchain/gcr/gcr_routines/handleNativeOperations.tssrc/libs/blockchain/gcr/gcr_routines/GCRBalanceRoutines.tssrc/features/networkUpgrade/{constants,safetyBounds}.tssrc/libs/blockchain/transaction.ts,src/model/entities/Transactions.ts,src/migrations/AddRpcAddressToTransactions.tsTests
Unit (281 pass, 1 pre-existing fail unrelated)
testing/forks/loadForkConfig.test.tstesting/forks/migrations/gasFeeSeparation.test.tstests/governance/calculateCurrentGas.test.tstests/governance/safetyBounds.test.tstests/blockchain/feeDistribution.test.tstests/blockchain/GCRBalanceRoutines.test.tstests/blockchain/handleNativeOperations.test.tsapplyGasFeeSeparation(extracted)tests/blockchain/applyGasFeeSeparation.test.ts128 DEM-665-specific tests across 8 suites. The 1 pre-existing fail is
tests/governance/snapshotWeightIntegrity.test.ts— Jest-mock setup issue, unrelated.Devnet rehearsal (both green)
Run command (PG env from
testing/devnet/.env):Both runs documented in
decimal_planning/REHEARSAL_RESULTS.mdRun 6 + Run 7.SDK companion
Published as
@kynesyslabs/demosdk@4.0.0. Breaking changes:TxFee: +rpc_address: string | nullRawTransaction: +rpcAddress: string | nullNetworkParameters: +additionalFeeand 7 distribution-percentage fields;burnFeeremovedNode
package.jsonpinned to4.0.0.Operator-facing changes
forks.gasFeeSeparationblock + treasury address requirements.Test plan
testing/forks/rehearsal/run-all.sh(10 scenarios sequential) — recommended for the canonical pre-deploy rehearsal cycle; deferred to opsactivationHeight— operator-side, per runbook §9Summary by CodeRabbit
New Features
Tests
Documentation
Chores