Upgradable network - #778
Merged
Merged
Conversation
Add colleague analysis on upgradable network configs (editable vs immutable parameters) and security conditions for upgrade transactions. https://claude.ai/code/session_017yrMzcSoPbeyWNL1Yb7C5g
Full design spec for the upgradable network mechanism — covers NetworkProperties schema, upgrade transaction type, stake-weighted voting, activation flow, implementation steps, and file plan. https://claude.ai/code/session_017yrMzcSoPbeyWNL1Yb7C5g
- Added `unstake_requested_at` and `unstake_available_at` fields to the Validators entity with appropriate indexing. - Renamed `staked` to `staked_amount` for clarity and updated its default value. - Introduced comprehensive tests for GCRValidatorStakeRoutines, covering stake, unstake, and exit operations. - Implemented integration tests for the handleStakingTx function to ensure proper transaction handling. - Created validatorHandlers to manage validator information retrieval and serialization. - Developed validatorsManagement to handle staking, unstaking, and exit transactions with validation checks.
- Implement tests for loading network parameters, ensuring defaults and upgrades are applied correctly. - Create safety bounds tests to validate proposal changes against defined limits. - Add snapshot weight integrity tests to confirm vote weight consistency during governance processes. - Develop tally upgrade votes tests to verify proposal outcomes based on voting weight. - Enhance staking integration tests to ensure proper handling of staking transactions and edits. - Update validators management tests to ensure minimum validator stake is correctly read from shared state.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughImplements Phase‑0 staking and Phase‑1 stackable‑genesis governance: adds DB entities, GCR routines for validator stake and upgrades/votes, safety bounds and dynamic fees, RPC/CLI/E2E tooling, validators schema changes, startup wiring for early parameter loading, and extensive tests and docs. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Node as Node (Tx Confirm)
participant GCR as GCR Routines
participant DB as Database
participant Chain as Chain (Block)
participant Shared as SharedState
Note over Client,Node: Submit networkUpgrade transaction
Client->>Node: Submit tx
Node->>Node: confirmTransaction -> runTypeDispatcher
Node->>GCR: validate proposal (handleGovernanceTx)
GCR->>DB: persist NetworkUpgrade (status = pending)
DB-->>GCR: saved
GCR-->>Node: validation success
Note over Chain,Node: At block/tallyBlock
Chain->>Node: insertBlock(currentBlock)
Node->>DB: persist block (in TX)
Node->>GCR: tallyUpgradeVotes(currentBlock) (in TX)
GCR->>DB: update NetworkUpgrade status -> activating/rejected
DB-->>Node: commit
Node->>Shared: loadNetworkParameters() (post-commit)
Shared->>Shared: merge active upgrades -> networkParameters/networkFee
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 20
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (23)
planning/security_conditions.md-3-5 (1)
3-5:⚠️ Potential issue | 🟡 MinorMake security conditions normative and measurable.
These bullets are currently too vague for implementation/audit. Define explicit threshold formulas, stake-weight basis, and “one pending upgrade” scope/enforcement.
Suggested wording upgrade
-- As we discussed, we can have some threshold to apply transactions -- Stake based approvals -- Every Validator can have Only 1 pending upgrade network transaction +- Upgrade transactions MUST be executed only if approval stake weight >= <defined_threshold>. +- Approval weight MUST be computed from validator staked amount snapshot at proposal start block. +- Each validator MUST be limited to exactly one pending network-upgrade proposal at a time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@planning/security_conditions.md` around lines 3 - 5, Update the three bullet items to be explicit and measurable: replace "some threshold to apply transactions" with a concrete approval threshold formula (e.g., required percentage P of total stake or N of active validators and how ties/quorums are computed), clarify "Stake based approvals" by specifying whether approvals are stake-weighted or validator-count-weighted, how stake is measured (active stake, delegated stake cutoff, snapshot timing), and define "Every Validator can have Only 1 pending upgrade network transaction" by specifying scope and enforcement (per-validator vs. per-operator, how pending is tracked, lifecycle transitions, and failure/timeout handling). Ensure each item names the exact metric/parameter (e.g., approvalThresholdPercent, stakeSnapshotEpoch, pendingUpgradeTTL) so they are normative and auditable..env.example-5-8 (1)
5-8:⚠️ Potential issue | 🟡 MinorReorder the new env keys to satisfy
dotenv-linter.
MIN_VALIDATOR_STAKEneeds to appear beforeNETWORK_FEE, andNETWORK_FEEneeds to appear beforeRPC_FEE; otherwise this file will keep triggering the ordering warning.♻️ Proposed reorder
CONSENSUS_TIME=10 -RPC_FEE=5 -NETWORK_FEE=10 # Minimum validator stake (raw bigint-as-string, must fit Postgres int64 ≤ 9.2e18) MIN_VALIDATOR_STAKE=1000000000000000000 +NETWORK_FEE=10 +RPC_FEE=5🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.env.example around lines 5 - 8, Reorder the environment variables so dotenv-linter's alphabetical ordering is satisfied: move MIN_VALIDATOR_STAKE to appear first, followed by NETWORK_FEE, and then RPC_FEE (i.e., MIN_VALIDATOR_STAKE, NETWORK_FEE, RPC_FEE); ensure the comment about MIN_VALIDATOR_STAKE remains adjacent to that key and no other keys are reordered.tests/staking/handleStakingTx.test.ts-62-62 (1)
62-62:⚠️ Potential issue | 🟡 MinorRemove unnecessary leading semicolons to satisfy ESLint.
Line 62, Line 88, Line 104, Line 117, and Line 127 trigger
no-extra-semi/@typescript-eslint/no-extra-semi.Suggested fix
- ;({ handleStakingTx } = await import( + ({ handleStakingTx } = await import( "@/libs/network/routines/transactions/handleStakingTx" )) ... - ;( + ( ValidatorsManagement.manageValidatorStakeTx as jest.Mock ).mockResolvedValue({ valid: true, message: "ok" } as never) ... - ;( + ( ValidatorsManagement.manageValidatorUnstakeTx as jest.Mock ).mockResolvedValue({ valid: true, message: "unstake ok" } as never) ... - ;( + ( ValidatorsManagement.manageValidatorExitTx as jest.Mock ).mockResolvedValue({ valid: true, message: "exit ok" } as never) ... - ;( + ( ValidatorsManagement.manageValidatorStakeTx as jest.Mock ).mockResolvedValue({ valid: false, message: "below minimum" } as never)Also applies to: 88-88, 104-104, 117-117, 127-127
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/staking/handleStakingTx.test.ts` at line 62, Remove the unnecessary leading semicolons before top-level parenthesized assignment/import expressions (e.g. change patterns like ;({ handleStakingTx } = await import(...)) to ({ handleStakingTx } = await import(...))) to satisfy ESLint no-extra-semi/@typescript-eslint/no-extra-semi; apply the same removal for the other occurrences that destructure or assign from dynamic imports in this test file (the lines that reference handleStakingTx and the similar parenthesized assignments at the other reported locations).planning/adversarial_review/staking_research/00_research_inventory.md-93-93 (1)
93-93:⚠️ Potential issue | 🟡 MinorHyphenate compound modifier for readability.
Line 93 should use a hyphenated form: “staking- or governance-related”.
Suggested fix
-**None are staking or governance related.** +**None are staking- or governance-related.**🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@planning/adversarial_review/staking_research/00_research_inventory.md` at line 93, Replace the unhyphenated compound modifier in the sentence "**None are staking or governance related.**" with the hyphenated form "**staking- or governance-related**" so the line reads "**None are staking- or governance-related.**"; update the phrase in the file planning/adversarial_review/staking_research/00_research_inventory.md accordingly.scripts/upgradable-network/gen-identity.ts-10-23 (1)
10-23:⚠️ Potential issue | 🟡 MinorFix outdated script path in help text/comments.
Line 10 and Line 22 still reference
scripts/devnet-gen-identity.ts, but this file isscripts/upgradable-network/gen-identity.ts.Suggested fix
- * bun scripts/devnet-gen-identity.ts .devnet/identity_1 + * bun scripts/upgradable-network/gen-identity.ts .devnet/identity_1 ... - "usage: bun scripts/devnet-gen-identity.ts <path>\n" + + "usage: bun scripts/upgradable-network/gen-identity.ts <path>\n" +🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/upgradable-network/gen-identity.ts` around lines 10 - 23, Update the hardcoded usage/help text in the main() function so it references the current file path/name instead of the outdated one: replace occurrences of "scripts/devnet-gen-identity.ts" with "scripts/upgradable-network/gen-identity.ts" (the string inside the console.error in main where target is checked) and ensure the usage line and any surrounding comments reflect the new script path.scripts/upgradable-network/gen-identity.ts-28-40 (1)
28-40:⚠️ Potential issue | 🟡 MinorOverwrite protection should include the
.pubsibling too.The script promises safe output behavior, but currently only
targetis guarded; an existing${target}.pubcan still be overwritten.Suggested fix
- if (existsSync(target)) { - console.error(`refusing to overwrite ${target}`) + if (existsSync(target) || existsSync(`${target}.pub`)) { + console.error( + `refusing to overwrite existing output (${target} or ${target}.pub)`, + ) process.exit(1) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/upgradable-network/gen-identity.ts` around lines 28 - 40, The script currently checks only existsSync(target) before writing the mnemonic and public key; update the pre-write check to also verify the sibling pub file (`${target}.pub`) and refuse to continue if either file exists. Locate the block that uses existsSync(target) and change it to test both target and `${target}.pub` (using the same existsSync) and call process.exit(1) with the error message mentioning both files; keep the later writeFileSync calls for writeFileSync(target, ...) and writeFileSync(`${target}.pub`, ...) and preserve the file modes (0o600 and 0o644).tests/governance/concurrentProposals.test.ts-112-123 (1)
112-123:⚠️ Potential issue | 🟡 MinorMake the repository mock honor the overlap predicate.
find()currently filters only bystatus, so these tests can still pass even if the production query stops checkingproposedParametersoverlap. That weakens the regression signal for the key-lock behavior you are trying to protect.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/governance/concurrentProposals.test.ts` around lines 112 - 123, The mock find() only filters by status; update it to also honor a proposedParameters overlap predicate by reading opts?.where?.proposedParameters?._overlap into a variable (e.g., allowedParams) and, when present, require that the row's proposedParameters array has at least one element in that allowedParams set; combine this intersection check with the existing status filter so both must pass when provided (use rows, r.proposedParameters and r.status to perform the checks).src/libs/network/handlers/validatorHandlers.ts-88-95 (1)
88-95:⚠️ Potential issue | 🟡 MinorReject non-finite block numbers before querying GCR.
extractBlockNumber()accepts any JavaScript number, soNaN,Infinity, and negative values can flow intogetGCRValidatorsAtBlock(...)and produce surprising results. Tighten this to an integer/range check before returning the handler response.Suggested fix
function extractBlockNumber(data: unknown): number | null { - if (typeof data === "number") return data + if (typeof data === "number" && Number.isInteger(data) && data >= 0) { + return data + } if (data && typeof data === "object") { const candidate = (data as { blockNumber?: unknown }).blockNumber - if (typeof candidate === "number") return candidate + if ( + typeof candidate === "number" && + Number.isInteger(candidate) && + candidate >= 0 + ) { + return candidate + } } return null }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libs/network/handlers/validatorHandlers.ts` around lines 88 - 95, extractBlockNumber currently returns any JS number allowing NaN/Infinity/negatives into getGCRValidatorsAtBlock; update extractBlockNumber to only return a valid block number by verifying Number.isFinite(candidate) (or Number.isSafeInteger), Number.isInteger(candidate) and candidate >= 0 (and optionally <= Number.MAX_SAFE_INTEGER or a configured max block), otherwise return null so the handler won't query GCR with invalid values; reference the function extractBlockNumber and the caller getGCRValidatorsAtBlock when making the change.planning/adversarial_review/staking_research/02_sdk_gap_analysis.md-22-40 (1)
22-40:⚠️ Potential issue | 🟡 MinorLabel the fenced example block.
The fence at Lines 22-40 has no language tag, which matches the MD040 warning and keeps docs lint noisy. Use
textortypescripthere.Suggested fix
-``` +```text🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@planning/adversarial_review/staking_research/02_sdk_gap_analysis.md` around lines 22 - 40, The fenced example block listing FooTransaction, FooPayload, TransactionContent, the TransactionSubtypes index exports, the DemosTransactions.foo builder, and the demosclass getFooStatus query is missing a language tag; update the opening fence to include a language (e.g., text or typescript) so the block is properly labeled (change ``` to ```text or ```typescript) to satisfy MD040 and silence the lint warning.tests/governance/tallyUpgradeVotes.test.ts-89-89 (1)
89-89:⚠️ Potential issue | 🟡 MinorResolve repeated
no-extra-semilint errors.Lines 89, 124, 151, 176, and 198 are all flagged by ESLint for unnecessary leading semicolons.
Also applies to: 124-124, 151-151, 176-176, 198-198
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/governance/tallyUpgradeVotes.test.ts` at line 89, Remove the unnecessary leading semicolons that are causing ESLint no-extra-semi errors—replace occurrences like ;(GCR.getGCRValidatorsAtBlock as jest.Mock).mockResolvedValue(...) and similar ;(...) prefixed statements with the same expressions without the leading semicolon; update the five test occurrences so they start directly with the expression (e.g., (GCR.getGCRValidatorsAtBlock as jest.Mock).mockResolvedValue) and re-run lint/format to ensure no other extra-semi instances remain.tests/governance/snapshotWeightIntegrity.test.ts-67-67 (1)
67-67:⚠️ Potential issue | 🟡 MinorFix
no-extra-semiviolations in dynamic-import and mock-cast lines.Lines 67, 247, and 281 use leading semicolons that ESLint marks as unnecessary.
Suggested fix pattern
- ;({ handleGovernanceTx } = await import( - "@/libs/network/routines/transactions/handleGovernanceTx" - )) + const governanceModule = await import( + "@/libs/network/routines/transactions/handleGovernanceTx" + ) + handleGovernanceTx = governanceModule.handleGovernanceTx- ;(Chain.getLastBlockNumber as jest.Mock).mockResolvedValue( + ;(Chain.getLastBlockNumber as jest.Mock).mockResolvedValue( VOTING_BLOCK as never, )For casted-call cases, either keep the current cast style if your lint config allows, or refactor through a temp variable typed as
jest.Mock.Also applies to: 247-247, 281-281
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/governance/snapshotWeightIntegrity.test.ts` at line 67, Remove the unnecessary leading semicolons used to guard dynamic imports and mock casts (e.g. the ";({ handleGovernanceTx } = await import(...))" at line referencing handleGovernanceTx and the two other similar lines), or refactor the casted-call pattern by assigning the imported value to a temporary const typed as jest.Mock before invoking it; specifically locate the dynamic import/mocked-call sites for handleGovernanceTx and the other two mock-cast occurrences and either delete the leading ";" or replace the inline cast-with-call with a temp variable typed as jest.Mock and call that variable to satisfy the linter.scripts/upgradable-network/sdk-builders.test.ts-11-12 (1)
11-12:⚠️ Potential issue | 🟡 MinorFix the usage path in the header comment.
The usage line references
bun scripts/test-sdk-builders.ts, but this file lives underscripts/upgradable-network/sdk-builders.test.ts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/upgradable-network/sdk-builders.test.ts` around lines 11 - 12, Update the header comment's usage line to point to the correct script path; replace the incorrect "bun scripts/test-sdk-builders.ts" usage with the actual path "bun scripts/upgradable-network/sdk-builders.test.ts" in the file-level comment near the top of scripts/upgradable-network/sdk-builders.test.ts so the documented invocation matches the file location.tests/governance/e2e.test.ts-103-103 (1)
103-103:⚠️ Potential issue | 🟡 MinorClean up
no-extra-semilint failures in mock/dynamic-import statements.Lines 103, 327, 331, and 341 are flagged by ESLint for unnecessary leading semicolons.
Also applies to: 327-327, 331-331, 341-341
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/governance/e2e.test.ts` at line 103, The tests contain unnecessary leading semicolons before dynamic import/destructuring expressions (e.g., the line that assigns handleStakingTx via ;({ handleStakingTx } = await import(...)) and the other dynamic-import lines flagged at 327, 331, 341); remove the leading semicolons and convert them to valid statements such as ({ handleStakingTx } = await import(...)) or assign to a const/let (const { handleStakingTx } = await import(...)) so the code parses without the extra semicolon while preserving the dynamic import behavior.src/libs/blockchain/routines/loadNetworkParameters.ts-56-56 (1)
56-56:⚠️ Potential issue | 🟡 MinorRemove the extra semicolon flagged by ESLint.
Line 56 is reported by
no-extra-semi/@typescript-eslint/no-extra-semi.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libs/blockchain/routines/loadNetworkParameters.ts` at line 56, Remove the unnecessary leading semicolon before the casted assignment in loadNetworkParameters.ts: replace the statement that currently begins with ";(params as unknown as Record<string, unknown>)[key] = value" by the same assignment without the extra semicolon so the offending no-extra-semi / `@typescript-eslint/no-extra-semi` error is resolved (target the assignment to (params as unknown as Record<string, unknown>)[key] = value).tests/governance/loadNetworkParameters.test.ts-49-49 (1)
49-49:⚠️ Potential issue | 🟡 MinorRemove the extra leading semicolon to satisfy ESLint.
Line 49 currently violates
no-extra-semi/@typescript-eslint/no-extra-semi.Suggested fix
beforeAll(async () => { - ;({ loadNetworkParameters } = await import( - "@/libs/blockchain/routines/loadNetworkParameters" - )) + const module = await import( + "@/libs/blockchain/routines/loadNetworkParameters" + ) + loadNetworkParameters = module.loadNetworkParameters })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/governance/loadNetworkParameters.test.ts` at line 49, Remove the extraneous leading semicolon before the dynamic import on the line that assigns loadNetworkParameters (the ";({ loadNetworkParameters } = await import(...)" expression); update the statement to start without the semicolon so it reads "({ loadNetworkParameters } = await import(...))" to satisfy ESLint rules (no-extra-semi / `@typescript-eslint/no-extra-semi`) while preserving the dynamic import and destructuring of loadNetworkParameters.planning/adversarial_review/staking_research/01_lead_conclusions.md-174-176 (1)
174-176:⚠️ Potential issue | 🟡 MinorAdd blank lines around tables to satisfy MD058.
markdownlint warns that these tables need blank lines before/after them.
Also applies to: 185-187, 196-198, 205-207
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@planning/adversarial_review/staking_research/01_lead_conclusions.md` around lines 174 - 176, Add a blank line before and after each Markdown table to satisfy MD058: locate the table that starts with "| File | Purpose |" (and the other similar tables in the same document) and insert one empty line above the table and one empty line below it so there is a separating blank line on both sides of every table.scripts/upgradable-network/e2e.sh-80-80 (1)
80-80:⚠️ Potential issue | 🟡 MinorPreflight currently masks missing
psql.
require psql || truemakes the check ineffective. Either enforce it or drop it from required tools to keep preflight behavior clear.🛠️ Suggested cleanup
-require docker; require curl; require jq; require bunx; require psql || true +require docker; require curl; require jq; require bunx🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/upgradable-network/e2e.sh` at line 80, The preflight line currently masks missing psql by using "require psql || true"; update that line so the check is deterministic: either remove "psql" from the required-tools list (so the line reads "require docker; require curl; require jq; require bunx;") or enforce psql by dropping the "|| true" and leaving "require psql" alongside the others; locate the line containing "require psql || true" and apply one of these two changes so missing psql is not silently ignored.planning/adversarial_review/stackable_genesis_system_v2.md-217-221 (1)
217-221:⚠️ Potential issue | 🟡 MinorAdd a language tag to the lifecycle diagram fence.
This plain fence keeps markdownlint flagging MD040;
textis sufficient here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@planning/adversarial_review/stackable_genesis_system_v2.md` around lines 217 - 221, The fenced lifecycle diagram is missing a language tag which triggers markdownlint MD040; update the code fence that contains the diagram (the block beginning with the triple backticks and the ASCII workflow "pending ──(tally: threshold met)──► approved ...") to include a language tag such as text (e.g., change ``` to ```text) so the fence is labeled and the linter warning is resolved.documentation/devs/upgradable-network-testing.md-133-154 (1)
133-154:⚠️ Potential issue | 🟡 MinorAdd a language tag to the tree listing fence.
The untyped block keeps markdownlint flagging MD040;
textis sufficient here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@documentation/devs/upgradable-network-testing.md` around lines 133 - 154, The fenced tree listing block is missing a language tag which triggers markdownlint MD040; update the opening fence from ``` to ```text so the tree listing is explicitly treated as plain text. Locate the triple-backtick fence that wraps the directory tree (the block shown with tests/, scripts/upgradable-network/, testing/devnet/, e2e-runs/) and add the language tag `text` to the opening fence.planning/stackable_genesis_system.md-69-71 (1)
69-71:⚠️ Potential issue | 🟡 MinorAdd a language tag to this fenced snippet.
This untyped fence triggers markdownlint MD040;
typescriptortextwould keep the doc lint-clean.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@planning/stackable_genesis_system.md` around lines 69 - 71, The fenced code snippet containing "{ proposalId, approve: true/false, voterPublicKey, signature }" needs a language tag to satisfy markdownlint MD040; update the opening fence from ``` to a language-tagged fence such as ```typescript (or ```text) so the snippet is typed—modify the fenced block that surrounds the proposal object to start with ```typescript.src/features/networkUpgrade/safetyBounds.ts-105-116 (1)
105-116:⚠️ Potential issue | 🟡 MinorReject non-plain
featureFlagspayloads.Arrays currently pass because they are objects with no entries, so malformed payloads like
featureFlags: []slip through validation.Suggested fix
function checkFeatureFlags( flags: Record<string, boolean> | undefined, ): { ok: true } | { ok: false; reason: string } { - if (!flags || typeof flags !== "object") { + if ( + !flags || + typeof flags !== "object" || + Array.isArray(flags) || + Object.getPrototypeOf(flags) !== Object.prototype + ) { return { ok: false, reason: "featureFlags must be an object" } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/networkUpgrade/safetyBounds.ts` around lines 105 - 116, The validation accepts arrays because typeof flags === "object" passes for arrays; update the featureFlags validation (the code using the variable "flags" in safetyBounds.ts) to explicitly reject non-plain objects by checking for null and arrays (e.g., if (flags === null || Array.isArray(flags)) return { ok: false, reason: "featureFlags must be an object" }); keep the existing Object.entries loop and boolean checks for values unchanged so only plain object maps with boolean values pass.scripts/upgradable-network/cli.ts-124-127 (1)
124-127:⚠️ Potential issue | 🟡 MinorValidate the
validatorsblock argument before formatting.
Number.parseInt()can yieldNaN, and the truthy check also hides block0; reject invalid input and use an explicitundefinedcheck for display.Suggested fix
async function cmdValidators(block?: string) { const demos = new Demos() await demos.connect(RPC_URL) const n = block ? Number.parseInt(block, 10) : undefined + if (block !== undefined && Number.isNaN(n)) { + exitWith(`invalid block number: ${block}`) + } const list = await demos.getValidators(n) - console.log(`${list.length} validator(s)${n ? ` @ block ${n}` : ""}:`) + console.log(`${list.length} validator(s)${n !== undefined ? ` @ block ${n}` : ""}:`) console.log(pretty(list)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/upgradable-network/cli.ts` around lines 124 - 127, The parsing of the CLI "block" arg can produce NaN and the current truthy check hides block 0; update the logic around block/n in cli.ts so you parse only when block is provided, validate with Number.isNaN and reject/exit on invalid input, pass the valid numeric (or undefined) to demos.getValidators, and change the display check to use n !== undefined (not a truthy check) when printing the "@ block" suffix; reference the variables block and n and the call to demos.getValidators and the pretty(list) output for where to apply these fixes.planning/adversarial_review/staking_research/plan/plan.md-10-20 (1)
10-20:⚠️ Potential issue | 🟡 MinorAdd language identifiers to fenced code blocks.
Lines [10] and [370] use unlabeled fences, which triggers MD040. Please tag these blocks (for example,
text).💡 Proposed fix
-``` +```text SDK Batch 1 (staking types) ↓ publish Node Batch 1 (staking backend) @@ Integration testing@@
-+text
SDK Batch 1 (v2.12.0)
├── ValidatorStakeTransaction.ts
├── ValidatorUnstakeTransaction.ts</details> Also applies to: 370-410 <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@planning/adversarial_review/staking_research/plan/plan.mdaround lines 10 -
20, The markdown contains unlabeled fenced code blocks (e.g., the blocks
starting with "SDK Batch 1 (staking types)" and "SDK Batch 1 (v2.12.0)" / the
block that lists ValidatorStakeTransaction.ts etc.) which triggers MD040; fix by
adding a language identifier to each fence (for example replace ``` withchange to the other unlabeled block around the integration testing section).
🧹 Nitpick comments (6)
myc.json (2)
15-16: Use one timestamp convention (prefer UTCZ) for manifest records.Mixing offsets (
+01:00/+02:00) andZincreases normalization overhead and can cause ordering inconsistencies if strings are compared directly. Standardizing to UTC ISO-8601 improves reliability.Proposed normalization example
- "created_at": "2026-04-11T15:49:00.295713817+02:00", - "updated_at": "2026-04-11T15:49:00.295713817+02:00" + "created_at": "2026-04-11T13:49:00.295713817Z", + "updated_at": "2026-04-11T13:49:00.295713817Z"Also applies to: 31-32, 47-48, 63-64
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@myc.json` around lines 15 - 16, Normalize all manifest timestamp fields to UTC ISO-8601 with a trailing Z by converting the "created_at" and "updated_at" string values (and the other occurrences noted) from offset timestamps like "+02:00" to UTC (e.g., "2026-04-11T13:49:00Z"); update each JSON record's "created_at" and "updated_at" properties (and the other similar pairs) so they use the same UTC Z convention consistently across the file, ensuring any generation code or tooling that produces these values also emits UTC/Z going forward.
11-11: Normalizetagsto arrays for consistency.
tagsvaries betweennull(lines 11, 27, 59) and a CSV string (line 43) in myc.json, creating unnecessary parsing complexity. Across the codebase, tags are consistently modeled as arrays (openapi-spec.json, Grafana dashboards, test fixtures). Use"tags": []/"tags": ["..."]uniformly.Affected lines
Lines 11, 27, 43, 59🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@myc.json` at line 11, Replace all occurrences of the "tags" property in myc.json so they are JSON arrays rather than null or CSV strings: change any "tags": null to "tags": [] and convert any CSV string like "tags": "a,b,c" to an array "tags": ["a","b","c"] (trim whitespace on each element). Ensure every object in myc.json uses the array form for the "tags" key so it matches the array-based model used elsewhere.src/model/entities/Validators.ts (1)
15-16: Prefer non-nullstaked_amountto enforce the staking invariant.Given this is the authoritative stake value, allowing
nullweakens correctness and increases defensive parsing across the codebase.Suggested schema tightening
- `@Column`("text", { name: "staked_amount", nullable: true, default: "0" }) - staked_amount: string | null + `@Column`("text", { name: "staked_amount", default: "0" }) + staked_amount: string🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/entities/Validators.ts` around lines 15 - 16, The Validators entity's staked_amount column should be non-nullable to enforce the staking invariant: update the `@Column` decorator on staked_amount by removing nullable: true (keep default: "0") and change the TypeScript type from string | null to string; ensure any code that constructed Validators without a staked_amount is updated to rely on the default and add a DB migration or schema update to alter the column to NOT NULL so the database and the entity definition remain consistent.tests/staking/handleStakingTx.test.ts (1)
116-124: Optional: assert exit handler invocation explicitly.Adding a
toHaveBeenCalledTimes(1)assertion formanageValidatorExitTxwould make the dispatch guarantee symmetric with the other route tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/staking/handleStakingTx.test.ts` around lines 116 - 124, Add an explicit invocation assertion for the exit handler: after calling handleStakingTx("validatorExit") in the test, assert that ValidatorsManagement.manageValidatorExitTx was called exactly once (e.g., using toHaveBeenCalledTimes(1)) so the test verifies both the returned result and that the manageValidatorExitTx route was invoked.src/libs/network/routines/transactions/handleStakingTx.ts (1)
32-35: Avoid coupling all branches tomanageValidatorStakeTx’s parameter type.
nodeTxis typed from one specific method and reused for unstake/exit calls. This can silently drift if those method signatures evolve independently.♻️ Suggested refactor
- const nodeTx = tx as unknown as Parameters< - typeof ValidatorsManagement.manageValidatorStakeTx - >[0] - switch (type) { case "validatorStake": { + const nodeTx = tx as Parameters< + typeof ValidatorsManagement.manageValidatorStakeTx + >[0] const payload = extractStakePayload(tx) if (!payload) { return { success: false, message: "Missing stake payload" } } const r = await ValidatorsManagement.manageValidatorStakeTx(nodeTx) @@ } case "validatorUnstake": { + const nodeTx = tx as Parameters< + typeof ValidatorsManagement.manageValidatorUnstakeTx + >[0] const r = await ValidatorsManagement.manageValidatorUnstakeTx( nodeTx, ) return { success: r.valid, message: r.message } } case "validatorExit": { + const nodeTx = tx as Parameters< + typeof ValidatorsManagement.manageValidatorExitTx + >[0] const r = await ValidatorsManagement.manageValidatorExitTx(nodeTx) return { success: r.valid, message: r.message } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libs/network/routines/transactions/handleStakingTx.ts` around lines 32 - 35, nodeTx is currently asserted to Parameters<typeof ValidatorsManagement.manageValidatorStakeTx>[0], coupling all branches to that single method signature; instead define and use a dedicated payload type (e.g., StakingTxPayload) that contains only the fields used by this routine or a union of the actual parameter types for stake/unstake/exit, and replace the current assertion on nodeTx with that new type (or import a shared type from ValidatorsManagement if available); update any calls that assume manageValidatorStakeTx's shape to use the new StakingTxPayload or the union so changes to individual method signatures won't silently break this handler.scripts/upgradable-network/e2e.sh (1)
248-248: Prefer explicitifoverA && B || Cin assertions.Use a normal
ifblock here to avoid SC2015 edge-case behavior and keep failure control flow unambiguous.♻️ Suggested change
-(( fee_ok == 1 )) && pass "live networkFee=${PROPOSED_FEE} on all 4 nodes" || { fail "live networkFee mismatch (see live-params.log)"; exit 5; } +if (( fee_ok == 1 )); then + pass "live networkFee=${PROPOSED_FEE} on all 4 nodes" +else + fail "live networkFee mismatch (see live-params.log)" + exit 5 +fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/upgradable-network/e2e.sh` at line 248, Replace the condensed conditional using shell's "&& ... || ..." with an explicit if/then/else block: check the variable fee_ok (used in the existing expression "(( fee_ok == 1 ))") with if [[ $fee_ok -eq 1 ]]; then call pass with the message "live networkFee=${PROPOSED_FEE} on all 4 nodes" else call fail with "live networkFee mismatch (see live-params.log)" and exit 5; keep the same pass, fail, PROPOSED_FEE and exit behavior but implement it as a clear if/else to avoid SC2015 edge-case semantics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8277cc3c-ba01-454a-8a3f-b110f25eb29b
📒 Files selected for processing (70)
.env.example.gitignoredocumentation/devs/upgradable-network-testing.mdmyc.jsonpackage.jsonplanning/adversarial_review/network_configs_editability_v2.mdplanning/adversarial_review/security_conditions_v2.mdplanning/adversarial_review/stackable_genesis_system_v2.mdplanning/adversarial_review/staking_research/00_research_inventory.mdplanning/adversarial_review/staking_research/01_lead_conclusions.mdplanning/adversarial_review/staking_research/02_sdk_gap_analysis.mdplanning/adversarial_review/staking_research/plan/plan.mdplanning/adversarial_review/staking_research/plan/spec.mdplanning/network_configs_editability.mdplanning/security_conditions.mdplanning/stackable_genesis_system.mdscripts/upgradable-network/cli.tsscripts/upgradable-network/e2e.shscripts/upgradable-network/gen-identity.tsscripts/upgradable-network/sdk-builders.test.tssrc/config/defaults.tssrc/config/envKeys.tssrc/config/loader.tssrc/config/types.tssrc/features/networkUpgrade/constants.tssrc/features/networkUpgrade/safetyBounds.tssrc/features/networkUpgrade/types.tssrc/features/staking/constants.tssrc/features/staking/types.tssrc/index.tssrc/libs/blockchain/chainBlocks.tssrc/libs/blockchain/gcr/gcr.tssrc/libs/blockchain/gcr/gcr_routines/GCRNetworkUpgradeRoutines.tssrc/libs/blockchain/gcr/gcr_routines/GCRValidatorStakeRoutines.tssrc/libs/blockchain/gcr/handleGCR.tssrc/libs/blockchain/routines/applyNetworkUpgrade.tssrc/libs/blockchain/routines/loadNetworkParameters.tssrc/libs/blockchain/routines/tallyUpgradeVotes.tssrc/libs/blockchain/routines/validateTransaction.tssrc/libs/blockchain/routines/validatorsManagement.tssrc/libs/network/dtr/dtrmanager.tssrc/libs/network/endpointExecution.tssrc/libs/network/handlers/governanceHandlers.tssrc/libs/network/handlers/index.tssrc/libs/network/handlers/validatorHandlers.tssrc/libs/network/routines/transactions/handleGovernanceTx.tssrc/libs/network/routines/transactions/handleStakingTx.tssrc/libs/utils/demostdlib/deriveMempoolOperation.tssrc/model/datasource.tssrc/model/entities/NetworkUpgrade.tssrc/model/entities/NetworkUpgradeVote.tssrc/model/entities/Validators.tssrc/types/demosdk-x-augmentations.d.tssrc/utilities/sharedState.tstesting/devnet/docker-compose.ymltests/governance/applyNetworkUpgrade.test.tstests/governance/concurrentProposals.test.tstests/governance/deriveFeesFromParameters.test.tstests/governance/e2e.test.tstests/governance/governanceHandlers.test.tstests/governance/handleGovernanceTx.test.tstests/governance/loadNetworkParameters.test.tstests/governance/safetyBounds.test.tstests/governance/snapshotWeightIntegrity.test.tstests/governance/tallyUpgradeVotes.test.tstests/staking/gcrValidatorStakeRoutines.test.tstests/staking/handleStakingTx.test.tstests/staking/integration.test.tstests/staking/validatorHandlers.test.tstests/staking/validatorsManagement.test.ts
… handling and fee calculations
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 15
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
♻️ Duplicate comments (4)
src/index.ts (1)
365-371:⚠️ Potential issue | 🟠 Major
loadNetworkParameters()is still not before all inbound traffic.Line 365’s guarantee is not true yet: RPC is already started at Line 303 in
warmup(), before Line 368 runs. That leaves a startup window where handlers can observe default params/fees. Move RPC listener startup until afterawait loadNetworkParameters()completes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.ts` around lines 365 - 371, The RPC listener is started in warmup() before governance is loaded, so move the RPC start call out of warmup() (or defer its invocation) and ensure it runs only after await loadNetworkParameters() completes; locate the RPC/server start logic inside warmup() and remove or guard its startup, then call/enable that RPC startup after loadNetworkParameters() and before peerBootstrap(indexState.PeerList) so that loadNetworkParameters(), findGenesisBlock() (if used), and any network parameter initialization always run before any inbound RPC handlers can execute.src/libs/blockchain/gcr/handleGCR.ts (1)
723-762:⚠️ Potential issue | 🔴 CriticalCritical: governance/stake writes still escape block transaction boundaries.
As noted in the new comment at Line 723, these branches persist through the default datasource. If block insertion fails later, stake/proposal/vote rows can remain committed without a confirmed block. Thread a
transactionalEntityManagerthroughapplyGCREditand these routines so writes share the same transaction as block commit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libs/blockchain/gcr/handleGCR.ts` around lines 723 - 762, The governance/stake branches (validatorStake, networkUpgrade, networkUpgradeVote) are currently using the default datasource and thus escape the block transaction; update applyGCREdit to accept and pass a transactionalEntityManager (e.g., transactionalEntityManager) into the routines and change the calls so GCRValidatorStakeRoutines.apply, GCRNetworkUpgradeRoutines.applyProposal and GCRNetworkUpgradeRoutines.applyVote receive and use that transactionalEntityManager for all DB persistence instead of the default datasource; adjust these routines' signatures to accept the manager and ensure all repository/entity operations within them use manager.getRepository / manager.save (or equivalent) so the writes participate in the same block transaction.src/libs/blockchain/routines/applyNetworkUpgrade.ts (1)
31-46:⚠️ Potential issue | 🟠 MajorKeep activation writes atomic to prevent partial state.
This still performs per-proposal saves in a plain loop. If one write fails mid-run, earlier proposals may already be moved to
active.Suggested fix
- const outcomes: ActivationOutcome[] = [] - for (const proposal of ready) { - const patch = proposal.proposedParameters ?? {} - proposal.status = "active" - await repo.save(proposal) - outcomes.push({ - proposalId: proposal.proposalId, - effectiveAtBlock: proposal.effectiveAtBlock, - applied: patch, - }) - log.info( - "GOVERNANCE", - `[activate] ${proposal.proposalId} (effectiveAtBlock=${proposal.effectiveAtBlock}): ${JSON.stringify(patch)}`, - ) - } - return outcomes + const applyWithRepo = async (txRepo: Repository<NetworkUpgrade>) => { + const outcomes: ActivationOutcome[] = [] + for (const proposal of ready) { + const patch = proposal.proposedParameters ?? {} + proposal.status = "active" + await txRepo.save(proposal) + outcomes.push({ + proposalId: proposal.proposalId, + effectiveAtBlock: proposal.effectiveAtBlock, + applied: patch, + }) + log.info( + "GOVERNANCE", + `[activate] ${proposal.proposalId} (effectiveAtBlock=${proposal.effectiveAtBlock}): ${JSON.stringify(patch)}`, + ) + } + return outcomes + } + + return repo.manager.transaction(async manager => { + const txRepo = manager.getRepository(NetworkUpgrade) + return applyWithRepo(txRepo) + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libs/blockchain/routines/applyNetworkUpgrade.ts` around lines 31 - 46, The loop updates each proposal individually which can leave partial activations if one save fails; change this to a single atomic operation using a transaction or batch save so all proposals are marked active together. Use the same symbols: gather the ready array, set proposal.status = "active" and prepare outcomes, then perform a single repo.manager.transaction (or repo.save(ready) inside a transaction) to persist all proposals, and only emit the log.info calls and return the ActivationOutcome[] after the transaction successfully commits so no partial updates are visible on failure.src/libs/blockchain/routines/loadNetworkParameters.ts (1)
23-33:⚠️ Potential issue | 🟠 MajorWrap repository acquisition in the same guarded fallback path as
find().If
Datasource.getInstance()orgetRepository(...)fails at Line 23–Line 26, the function throws and never reaches the genesis-default fallback path.🛡️ Suggested fix
- if (!resolvedRepo) { - const db = await Datasource.getInstance() - resolvedRepo = db.getDataSource().getRepository(NetworkUpgrade) - } - let active: NetworkUpgrade[] = [] try { + if (!resolvedRepo) { + const db = await Datasource.getInstance() + resolvedRepo = db.getDataSource().getRepository(NetworkUpgrade) + } active = await resolvedRepo.find({ where: { status: "active" }, order: { effectiveAtBlock: "ASC", proposalId: "ASC" }, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libs/blockchain/routines/loadNetworkParameters.ts` around lines 23 - 33, The repository acquisition for NetworkUpgrade is not guarded, so if Datasource.getInstance() or db.getDataSource().getRepository(NetworkUpgrade) throws the function will exit before the genesis-default fallback; wrap the resolvedRepo assignment in the same try/catch (or conditional fallback) used around the resolvedRepo.find() call in loadNetworkParameters so that failures to get the datasource/repository are caught and the code proceeds to the genesis-default path (catch exceptions from Datasource.getInstance and getRepository and set resolvedRepo to undefined or handle accordingly before continuing).
🟡 Minor comments (25)
scripts/upgradable-network/gen-identity.ts-3-11 (1)
3-11:⚠️ Potential issue | 🟡 MinorUpdate the help text to match the new script path.
The comment and usage string still point at
scripts/devnet-gen-identity.ts, but this file now lives underscripts/upgradable-network/, so the CLI help is stale.🛠️ Suggested update
- * bun scripts/devnet-gen-identity.ts .devnet/identity_1 + * bun scripts/upgradable-network/gen-identity.ts .devnet/identity_1 ... - "usage: bun scripts/devnet-gen-identity.ts <path>\n" + + "usage: bun scripts/upgradable-network/gen-identity.ts <path>\n" +Also applies to: 22-23
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/upgradable-network/gen-identity.ts` around lines 3 - 11, Update the top comment and any CLI usage strings that still reference "scripts/devnet-gen-identity.ts" to the new path "scripts/upgradable-network/gen-identity.ts"; specifically replace the example invocation line containing "bun scripts/devnet-gen-identity.ts .devnet/identity_1" so the help text and usage reflect the current file location and correct invocation (e.g., "bun scripts/upgradable-network/gen-identity.ts ..."), ensuring both the block comment and any runtime help/usage variable or string are changed (look for the literal "scripts/devnet-gen-identity.ts" in this file)..gitignore-87-90 (1)
87-90:⚠️ Potential issue | 🟡 MinorAlso ignore the generated
.pubsidecar.
.manual-test-mnemoniconly covers the mnemonic file itself; the identity generator also writes<path>.pub, so.manual-test-mnemonic.pubwill still show up as an untracked artifact.🛠️ Suggested update
.manual-test-mnemonic +.manual-test-mnemonic.pub🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gitignore around lines 87 - 90, The .gitignore entry for the generated local devnet mnemonic needs to also ignore the public sidecar; update the ignore rules so the identity generator's "<path>.pub" is covered (for example add an entry that matches .manual-test-mnemonic.pub or use a wildcard like .manual-test-mnemonic*), ensuring the public key file produced alongside .manual-test-mnemonic is not treated as an untracked artifact.scripts/upgradable-network/sdk-builders.test.ts-104-111 (1)
104-111:⚠️ Potential issue | 🟡 MinorUse a stricter empty-object assertion for
unstake().
JSON.stringify(p) === "{}"can still pass for objects that carry onlyundefinedproperties. A key-count check is more precise here.🔧 Suggested fix
const p = (tx?.content?.data?.[1] ?? {}) as any check( "unstake payload is empty object", - JSON.stringify(p) === "{}", + Object.keys(p).length === 0, p, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/upgradable-network/sdk-builders.test.ts` around lines 104 - 111, Replace the loose JSON.stringify-based empty-object check for the unstake payload with a strict key-count assertion: ensure the variable p (from DemosTransactions.unstake) is an object and assert Object.keys(p).length === 0 (e.g., check("unstake payload is empty object", p && typeof p === "object" && Object.keys(p).length === 0, p)). This uses the existing p variable and keeps assertShape("unstake()", tx, "validatorUnstake", owner) intact.scripts/upgradable-network/sdk-builders.test.ts-11-13 (1)
11-13:⚠️ Potential issue | 🟡 MinorFix the usage string to point at this script.
The header references
scripts/test-sdk-builders.ts, but this file lives atscripts/upgradable-network/sdk-builders.test.ts. Copy/pasting the documented command will fail.🔧 Suggested fix
- * Usage: bun scripts/test-sdk-builders.ts + * Usage: bun scripts/upgradable-network/sdk-builders.test.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/upgradable-network/sdk-builders.test.ts` around lines 11 - 13, Update the top-of-file usage string so it points to the actual script location: replace the incorrect "bun scripts/test-sdk-builders.ts" text with "bun scripts/upgradable-network/sdk-builders.test.ts" (search for the existing usage comment string in this file and update it accordingly) so copy/pasting the documented command runs the right script..env.example-4-8 (1)
4-8:⚠️ Potential issue | 🟡 MinorReorder the new env keys to satisfy dotenv-linter.
The added keys are out of the expected order, so this will trigger the
UnorderedKeywarning.🔧 Suggested fix
CONSENSUS_TIME=10 +MIN_VALIDATOR_STAKE=1000000000000000000 +NETWORK_FEE=10 RPC_FEE=5 -NETWORK_FEE=10 -# Minimum validator stake (raw bigint-as-string, must fit Postgres int64 ≤ 9.2e18) -MIN_VALIDATOR_STAKE=1000000000000000000 +# Minimum validator stake (raw bigint-as-string, must fit Postgres int64 ≤ 9.2e18)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.env.example around lines 4 - 8, Reorder the new environment keys to satisfy dotenv-linter's UnorderedKey rule: currently the block lists CONSENSUS_TIME, RPC_FEE, NETWORK_FEE, MIN_VALIDATOR_STAKE; sort them alphabetically so the final order is CONSENSUS_TIME, MIN_VALIDATOR_STAKE, NETWORK_FEE, RPC_FEE (referencing the keys CONSENSUS_TIME, RPC_FEE, NETWORK_FEE, MIN_VALIDATOR_STAKE to locate the section).tests/staking/validatorHandlers.test.ts-38-41 (1)
38-41:⚠️ Potential issue | 🟡 MinorRemove the leading semicolons in this suite.
These statements are currently tripping
no-extra-semi/@typescript-eslint/no-extra-semi, so the new tests will fail lint until the extra semicolons are dropped.Also applies to: 71-73, 89-91, 100-102, 121-124, 144-146, 156-158, 167-169, 178-180
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/staking/validatorHandlers.test.ts` around lines 38 - 41, Remove the unnecessary leading semicolons that precede dynamic import destructuring expressions (e.g., the `;({ validatorHandlers } = await import("@/libs/network/handlers/validatorHandlers"))` pattern) throughout the test file — delete the extra semicolons so the statements begin directly with `({ validatorHandlers } = await import(...))` (and the equivalent for the other occurrences at the noted locations) to satisfy the no-extra-semi/@typescript-eslint:no-extra-semi linter rule.tests/governance/deriveFeesFromParameters.test.ts-44-47 (1)
44-47:⚠️ Potential issue | 🟡 MinorRemove the extra semicolon before the dynamic import.
ESLint is already flagging this block with
no-extra-semi, so the new suite will fail lint as written.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/governance/deriveFeesFromParameters.test.ts` around lines 44 - 47, Remove the stray semicolon before the dynamic import that assigns resolveDynamicFees; specifically edit the beforeAll block where resolveDynamicFees is set via the dynamic import of "@/libs/utils/demostdlib/deriveMempoolOperation" so the line starts directly with ({ resolveDynamicFees } = await import(...)) rather than ;({ ... to satisfy ESLint no-extra-semi and ensure the test suite passes linting.tests/staking/handleStakingTx.test.ts-136-143 (1)
136-143:⚠️ Potential issue | 🟡 MinorAssert the other handlers stay untouched on unknown types.
This only checks
manageValidatorStakeTx, so a regression tomanageValidatorUnstakeTxormanageValidatorExitTxcould still slip through.Suggested assertion update
expect( ValidatorsManagement.manageValidatorStakeTx, ).not.toHaveBeenCalled() + expect( + ValidatorsManagement.manageValidatorUnstakeTx, + ).not.toHaveBeenCalled() + expect( + ValidatorsManagement.manageValidatorExitTx, + ).not.toHaveBeenCalled()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/staking/handleStakingTx.test.ts` around lines 136 - 143, The test for unknown staking types currently only asserts that manageValidatorStakeTx wasn't called; update the spec to also assert that manageValidatorUnstakeTx and manageValidatorExitTx are not invoked when calling handleStakingTx(tx("validatorBogus")). Locate the test block that calls handleStakingTx and add negative expectations for ValidatorsManagement.manageValidatorUnstakeTx and ValidatorsManagement.manageValidatorExitTx (similar to the existing not.toHaveBeenCalled() assertion) so all validator handler functions remain untouched on unknown types.planning/adversarial_review/staking_research/02_sdk_gap_analysis.md-22-98 (1)
22-98:⚠️ Potential issue | 🟡 MinorAdd language tags to the fenced examples.
Every fenced block in this doc is unlabeled, so markdownlint will keep flagging it. Use
textfor the pseudo-code examples, ortypescriptwhere the snippet is actual TS.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@planning/adversarial_review/staking_research/02_sdk_gap_analysis.md` around lines 22 - 98, The markdown has unlabeled fenced code blocks causing markdownlint warnings; update each fenced block in 02_sdk_gap_analysis.md to include an appropriate language tag (use "typescript" for the real TypeScript examples such as the Transaction builders and types like DemosTransactions.stake, Demos.getValidatorInfo, GCREditValidatorStake, and NetworkParameters, and use "text" for pseudo-code or plain lists like the Phase 0/Phase 1 enumerations), ensuring every ``` fence becomes either ```typescript or ```text as appropriate so linting passes.tests/governance/snapshotWeightIntegrity.test.ts-240-278 (1)
240-278:⚠️ Potential issue | 🟡 MinorActually change the validator snapshot before tallying.
The first scenario never changes the mocked validator stake between vote and tally, so it doesn't really exercise the "unstake before tally" case. If
tallyUpgradeVotesever starts re-reading validator state, this test could still pass. Consider switching the GCR mock toPOST_UNSTAKE_STAKEbeforetallyUpgradeVotes.Suggested tweak
await applyEditsTo(voteTxObj) expect(voteRepo._votes).toHaveLength(1) expect(voteRepo._votes[0].weight).toBe(SNAPSHOT_STAKE) + + ;(GCR.getGCRValidatorsAtBlock as jest.Mock).mockResolvedValueOnce([ + { address: VOTER, staked_amount: POST_UNSTAKE_STAKE }, + ] as never)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/governance/snapshotWeightIntegrity.test.ts` around lines 240 - 278, The test currently keeps GCR.getGCRValidatorsAtBlock mocked to return SNAPSHOT_STAKE for both vote and tally; update the test so after applyEditsTo(voteTxObj) (or before calling tallyUpgradeVotes) you reset/mock GCR.getGCRValidatorsAtBlock to return POST_UNSTAKE_STAKE to simulate the validator unstaking between vote and tally, then call tallyUpgradeVotes and assert that the persisted vote weight (voteRepo._votes[0].weight) remains SNAPSHOT_STAKE while the re-read validator state shows POST_UNSTAKE_STAKE; reference GCR.getGCRValidatorsAtBlock, SNAPSHOT_STAKE, POST_UNSTAKE_STAKE, applyEditsTo, and tallyUpgradeVotes to locate and modify the test.src/libs/network/handlers/validatorHandlers.ts-1-5 (1)
1-5:⚠️ Potential issue | 🟡 MinorAlign logger import with other imports in this file.
Line 4 uses
src/utilities/loggerwhile lines 1–3 all use the@/alias. For consistency within this file, use@/utilities/logger.Suggested fix
-import log from "src/utilities/logger" +import log from "@/utilities/logger"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libs/network/handlers/validatorHandlers.ts` around lines 1 - 5, Update the logger import to use the same "@/” alias as the other imports: replace the current import of "src/utilities/logger" that assigns to the symbol log with "@/utilities/logger". Ensure the imported identifier remains log so usages in this file (e.g., within validatorHandlers) continue to work without further changes.tests/governance/e2e.test.ts-103-103 (1)
103-103:⚠️ Potential issue | 🟡 MinorClean up extra semicolons causing lint failures.
Lines 103, 327, 331, and 341 use unnecessary leading semicolons flagged by ESLint.
Suggested fix
- ;({ handleStakingTx } = await import( + ({ handleStakingTx } = await import( "@/libs/network/routines/transactions/handleStakingTx" )) @@ - ;(Chain.getLastBlockNumber as jest.Mock).mockResolvedValue(n as never) + (Chain.getLastBlockNumber as jest.Mock).mockResolvedValue(n as never) @@ - ;(GCR.getGCRValidatorStatus as jest.Mock).mockImplementation( + (GCR.getGCRValidatorStatus as jest.Mock).mockImplementation( @@ - ;(GCR.getGCRValidatorsAtBlock as jest.Mock).mockResolvedValue( + (GCR.getGCRValidatorsAtBlock as jest.Mock).mockResolvedValue(Also applies to: 327-327, 331-331, 341-341
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/governance/e2e.test.ts` at line 103, Remove the stray leading semicolons that cause ESLint failures by deleting the unnecessary semicolons at the start of statements that begin with a parenthesis; specifically remove the leading ";" before the dynamic import assignment "({ handleStakingTx } = await import(...))" and the other occurrences that start with "(" in the same file so the statements start with "(" or the identifier directly (e.g., convert ";({ handleStakingTx } = await import(...))" to "({ handleStakingTx } = await import(...))").tests/governance/tallyUpgradeVotes.test.ts-89-89 (1)
89-89:⚠️ Potential issue | 🟡 MinorRemove unnecessary leading semicolons (
no-extra-semi).Lines 89, 124, 151, 176, and 198 contain extra semicolons that are currently lint errors.
Suggested fix
- ;(GCR.getGCRValidatorsAtBlock as jest.Mock).mockResolvedValue([ + (GCR.getGCRValidatorsAtBlock as jest.Mock).mockResolvedValue([ @@ - ;(GCR.getGCRValidatorsAtBlock as jest.Mock).mockResolvedValue([ + (GCR.getGCRValidatorsAtBlock as jest.Mock).mockResolvedValue([ @@ - ;(GCR.getGCRValidatorsAtBlock as jest.Mock).mockResolvedValue( + (GCR.getGCRValidatorsAtBlock as jest.Mock).mockResolvedValue( @@ - ;(GCR.getGCRValidatorsAtBlock as jest.Mock).mockResolvedValue([ + (GCR.getGCRValidatorsAtBlock as jest.Mock).mockResolvedValue([ @@ - ;(GCR.getGCRValidatorsAtBlock as jest.Mock).mockResolvedValue([ + (GCR.getGCRValidatorsAtBlock as jest.Mock).mockResolvedValue([Also applies to: 124-124, 151-151, 176-176, 198-198
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/governance/tallyUpgradeVotes.test.ts` at line 89, Remove the unnecessary leading semicolons that cause the no-extra-semi lint errors by deleting the stray semicolons preceding the mock calls; specifically remove the leading ";" before expressions such as (GCR.getGCRValidatorsAtBlock as jest.Mock).mockResolvedValue(...) and the analogous leading semicolons at the other occurrences in this test file so the mock calls and statements begin directly with the expression.tests/governance/concurrentProposals.test.ts-81-81 (1)
81-81:⚠️ Potential issue | 🟡 MinorRemove extra semicolon flagged by ESLint.
Line 81 has an unnecessary leading semicolon (
no-extra-semi/@typescript-eslint/no-extra-semi).Suggested fix
-beforeAll(async () => { - ;({ handleGovernanceTx } = await import( +beforeAll(async () => { + ({ handleGovernanceTx } = await import( "@/libs/network/routines/transactions/handleGovernanceTx" )) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/governance/concurrentProposals.test.ts` at line 81, Remove the unnecessary leading semicolon before the dynamic import expression that assigns handleGovernanceTx: change the line starting with ";({ handleGovernanceTx } = await import(" by deleting the extra semicolon so the statement begins with "({ handleGovernanceTx } = await import(...". This keeps the destructuring assignment of handleGovernanceTx intact and resolves the ESLint `@typescript-eslint/no-extra-semi` error.tests/governance/loadNetworkParameters.test.ts-49-49 (1)
49-49:⚠️ Potential issue | 🟡 MinorRemove the unnecessary leading semicolon before the dynamic import assignment.
Line 49 triggers
no-extra-semi/@typescript-eslint/no-extra-semi.🧹 Suggested fix
- ;({ loadNetworkParameters } = await import( + ({ loadNetworkParameters } = await import( "@/libs/blockchain/routines/loadNetworkParameters" ))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/governance/loadNetworkParameters.test.ts` at line 49, Remove the unnecessary leading semicolon before the dynamic import assignment for loadNetworkParameters; update the line starting with ;({ loadNetworkParameters } = await import(...) to simply ({ loadNetworkParameters } = await import(...)) so the statement no longer contains an extra semicolon and satisfies no-extra-semi / `@typescript-eslint/no-extra-semi`.planning/stackable_genesis_system.md-69-71 (1)
69-71:⚠️ Potential issue | 🟡 MinorAdd a language identifier to the fenced code block.
This block currently violates
MD040and will keep markdown lint noisy.📝 Suggested fix
- ``` + ```json { proposalId, approve: true/false, voterPublicKey, signature } ```🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@planning/stackable_genesis_system.md` around lines 69 - 71, Add a language identifier to the fenced code block containing "{ proposalId, approve: true/false, voterPublicKey, signature }" to satisfy MD040; update the opening fence from ``` to ```json so the block is recognized as JSON (e.g., change the fence that precedes the object literal in the proposal example).src/libs/blockchain/routines/loadNetworkParameters.ts-56-56 (1)
56-56:⚠️ Potential issue | 🟡 MinorRemove extra leading semicolon in assignment.
Line 56 triggers
no-extra-semi/@typescript-eslint/no-extra-semi.🧹 Suggested fix
- ;(params as unknown as Record<string, unknown>)[key] = value + (params as unknown as Record<string, unknown>)[key] = value🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libs/blockchain/routines/loadNetworkParameters.ts` at line 56, Remove the stray leading semicolon before the assignment inside loadNetworkParameters; change the statement that currently reads with ";(params as unknown as Record<string, unknown>)[key] = value" to a normal assignment without the extra semicolon so the linter no-extra-semi / `@typescript-eslint/no-extra-semi` error is resolved while keeping the existing cast of params, key and value intact.planning/adversarial_review/stackable_genesis_system_v2.md-217-221 (1)
217-221:⚠️ Potential issue | 🟡 MinorSpecify a language for the status-lifecycle fenced block.
This currently trips
MD040and should be labeled (for example,text).📝 Suggested fix
-``` +```text pending ──(tally: threshold met)──► approved ──(grace period)──► activating ──(effectiveAtBlock)──► active │ └──(tally: threshold not met)──► rejected🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@planning/adversarial_review/stackable_genesis_system_v2.md` around lines 217 - 221, The fenced status-lifecycle block showing the state diagram is missing a language label, which triggers MD040; update the triple-backtick fence that wraps the diagram (the "status-lifecycle" fenced block containing the lines starting with "pending ──(tally: threshold met)──► approved...") to include a language identifier such as text (i.e., change ``` to ```text) so the block is explicitly labeled and the lint rule is satisfied.tests/staking/validatorsManagement.test.ts-270-281 (1)
270-281:⚠️ Potential issue | 🟡 MinorMake the “before lock elapsed” assertion deterministic against config changes.
Using a fixed block height (
500) can accidentally become post-lock ifUNSTAKE_LOCK_BLOCKSchanges. Tie the mocked current block tounstake_available_at - 1instead.✅ Suggested fix
- jest.mocked(GCR.getGCRValidatorStatus).mockResolvedValue({ + const availableAt = 10 + UNSTAKE_LOCK_BLOCKS + jest.mocked(GCR.getGCRValidatorStatus).mockResolvedValue({ address: SENDER, status: VALIDATOR_STATUS_UNSTAKING, unstake_requested_at: 10, - unstake_available_at: 10 + UNSTAKE_LOCK_BLOCKS, + unstake_available_at: availableAt, } as never) - jest.mocked(Chain.getLastBlockNumber).mockResolvedValue(500 as never) + jest.mocked(Chain.getLastBlockNumber).mockResolvedValue( + (availableAt - 1) as never, + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/staking/validatorsManagement.test.ts` around lines 270 - 281, The test should compute the mocked validator status and use its unstake_available_at to set the current block deterministically instead of a hardcoded 500; create a status object (with unstake_requested_at and unstake_available_at = 10 + UNSTAKE_LOCK_BLOCKS), mock GCR.getGCRValidatorStatus with that status, then mock Chain.getLastBlockNumber to return status.unstake_available_at - 1 before calling ValidatorsManagement.manageValidatorExitTx so the "Lock not elapsed" assertion remains correct even if UNSTAKE_LOCK_BLOCKS changes.tests/staking/integration.test.ts-64-67 (1)
64-67:⚠️ Potential issue | 🟡 MinorRemove the unnecessary block-leading semicolons.
These hit the same
no-extra-semilint rule and will keep the test file from being lint-clean.Suggested fix
beforeAll(async () => { - ;({ handleStakingTx } = await import( + ({ handleStakingTx } = await import( "@/libs/network/routines/transactions/handleStakingTx" )) @@ - ;(GCR.getGCRValidatorStatus as jest.Mock).mockResolvedValue( + (GCR.getGCRValidatorStatus as jest.Mock).mockResolvedValue( null as never, ) @@ - ;(GCR.getGCRValidatorStatus as jest.Mock).mockResolvedValue( + (GCR.getGCRValidatorStatus as jest.Mock).mockResolvedValue( null as never, ) @@ - ;(GCR.getGCRValidatorStatus as jest.Mock).mockResolvedValue( + (GCR.getGCRValidatorStatus as jest.Mock).mockResolvedValue( null as never, ) @@ - ;(GCR.getGCRValidatorStatus as jest.Mock).mockResolvedValue( + (GCR.getGCRValidatorStatus as jest.Mock).mockResolvedValue( null as never, )Also applies to: 212-215, 247-250, 316-319, 325-328
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/staking/integration.test.ts` around lines 64 - 67, Remove the unnecessary leading semicolons before dynamic destructuring imports (they trigger the no-extra-semi lint error); locate the beforeAll block that does ";({ handleStakingTx } = await import(...))" and any other similar patterns and simply delete the leading ";" so the statement begins with "({ handleStakingTx } = await import(...))" (apply the same change for the other destructured dynamic imports in the file).planning/adversarial_review/staking_research/plan/plan.md-10-20 (1)
10-20:⚠️ Potential issue | 🟡 MinorAdd fence languages to the ASCII diagrams.
Both blocks currently fail
markdownlint(MD040).Suggested fix
-``` +```text SDK Batch 1 (staking types) ↓ publish Node Batch 1 (staking backend) @@ Integration testing@@
-+text
SDK Batch 1 (v2.12.0)
├── ValidatorStakeTransaction.ts
├── ValidatorUnstakeTransaction.ts
@@
Integration TestingAlso applies to: 370-410
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@planning/adversarial_review/staking_research/plan/plan.md` around lines 10 - 20, The ASCII diagram code blocks in the document are missing language fences and fail markdownlint MD040; update each triple-backtick block (the diagrams beginning with "SDK Batch 1 (staking types)" and the later block listing files like "ValidatorStakeTransaction.ts" and the other block around lines 370-410) to include the "text" fence (i.e., change ``` to ```text) so the diagrams are treated as plain text code blocks and MD040 is satisfied; ensure every fenced block in the file that contains ASCII art or plain lists uses ```text consistently.scripts/upgradable-network/cli.ts-5-6 (1)
5-6:⚠️ Potential issue | 🟡 MinorUpdate the copy-paste examples to this script’s real path.
The banner/help text still points to
scripts/manual_test_upgradable_network.ts, but this file lives atscripts/upgradable-network/cli.ts. Users following the examples will hit a missing-file error.Suggested fix
- * tsx scripts/manual_test_upgradable_network.ts <command> [args] + * tsx scripts/upgradable-network/cli.ts <command> [args] @@ - tsx scripts/manual_test_upgradable_network.ts <command> [args] + tsx scripts/upgradable-network/cli.ts <command> [args] @@ - tsx scripts/manual_test_upgradable_network.ts propose networkFee 12 - tsx scripts/manual_test_upgradable_network.ts vote <proposalId> yes - tsx scripts/manual_test_upgradable_network.ts votes <proposalId> - tsx scripts/manual_test_upgradable_network.ts params # after effectiveAtBlock + tsx scripts/upgradable-network/cli.ts propose networkFee 12 + tsx scripts/upgradable-network/cli.ts vote <proposalId> yes + tsx scripts/upgradable-network/cli.ts votes <proposalId> + tsx scripts/upgradable-network/cli.ts params # after effectiveAtBlockAlso applies to: 260-295
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/upgradable-network/cli.ts` around lines 5 - 6, Update the help/banner example strings that still reference "tsx scripts/manual_test_upgradable_network.ts <command> [args]" to use this script's actual path "tsx scripts/upgradable-network/cli.ts <command> [args]"; search for that example literal (and any other copy-pasted occurrences in the same file) and replace them so all usage examples and help text point to the correct script path.tests/governance/handleGovernanceTx.test.ts-85-89 (1)
85-89:⚠️ Potential issue | 🟡 MinorDrop the block-leading semicolons to satisfy ESLint.
These statements are the first expression in their block, so the defensive
;isn't needed here and currently failsno-extra-semi.Suggested fix
beforeAll(async () => { - ;({ handleGovernanceTx } = await import( + ({ handleGovernanceTx } = await import( "@/libs/network/routines/transactions/handleGovernanceTx" )) }) @@ - ;(GCR.getGCRValidatorStatus as jest.Mock).mockResolvedValue( + (GCR.getGCRValidatorStatus as jest.Mock).mockResolvedValue( null as never, ) @@ - ;(Chain.getLastBlockNumber as jest.Mock).mockResolvedValue( + (Chain.getLastBlockNumber as jest.Mock).mockResolvedValue( (TALLY + 1) as never, ) @@ - ;(GCR.getGCRValidatorsAtBlock as jest.Mock).mockResolvedValue([ + (GCR.getGCRValidatorsAtBlock as jest.Mock).mockResolvedValue([ { address: "other", staked_amount: "100" }, ] as never) @@ - ;(Chain.getLastBlockNumber as jest.Mock).mockResolvedValue( + (Chain.getLastBlockNumber as jest.Mock).mockResolvedValue( 900 as never, )Also applies to: 170-173, 294-297, 303-306, 362-367
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/governance/handleGovernanceTx.test.ts` around lines 85 - 89, Remove the unnecessary leading semicolons before the parenthesized destructuring import expressions (e.g., the statement that assigns handleGovernanceTx via ;({ handleGovernanceTx } = await import(...))) — these are the first expressions in their blocks so the defensive semicolon triggers ESLint no-extra-semi; simply delete the leading semicolon in those beforeAll/other block statements (and apply the same change to the other similar parenthesized-destructuring import lines).documentation/devs/upgradable-network-testing.md-133-154 (1)
133-154:⚠️ Potential issue | 🟡 MinorAdd a language tag to the directory-tree fence.
This block trips
markdownlint(MD040), so the docs file won't stay lint-clean as written.Suggested fix
-``` +```text tests/ ├── governance/ # Phase 1 unit suites (10 files) └── staking/ # Phase 0 unit suites (5 files) @@ e2e-runs/ # gitignored, one dir per run └── <UTC-timestamp>/ ├── SUMMARY.txt └── *.log</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@documentation/devs/upgradable-network-testing.mdaround lines 133 - 154, The
fenced directory-tree block currently has no language tag and triggers
markdownlint MD040; update the opening fence to include a language tag (for
example changetotext) so the block becomes a labeled code fence; make
this change in the directory-tree block shown in the
upgradable-network-testing.md file so the tree listing is fenced as ```text ...planning/adversarial_review/staking_research/01_lead_conclusions.md-173-207 (1)
173-207:⚠️ Potential issue | 🟡 MinorFix MD058 table spacing in the file.
Several tables are missing required blank lines around them, which will keep markdownlint warnings active.
Suggested fix pattern (apply to all listed tables)
### Node — Files to Create + | File | Purpose | |------|---------| | `src/libs/network/handlers/validatorHandlers.ts` | RPC handlers for validator queries | ... | `src/libs/blockchain/routines/applyNetworkUpgrade.ts` | Activation (Phase 1) | + ### Node — Files to Modify🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@planning/adversarial_review/staking_research/01_lead_conclusions.md` around lines 173 - 207, Add the required blank Markdown lines before and after each table to satisfy MD058: ensure there is a blank line above and below every table block under the headings "Node — Files to Create", "Node — Files to Modify", "SDK — Files to Create", and "SDK — Files to Modify" in 01_lead_conclusions.md so each table is separated from the surrounding text/headers; update every listed table instance accordingly so markdownlint no longer reports MD058.
🧹 Nitpick comments (2)
myc.json (2)
15-16: Consider normalizing timestamps to UTCZformat.Mixed timezone offsets and
Zare all valid ISO-8601, but normalizing to UTC improves deterministic diffs and simpler downstream sorting/comparisons.Also applies to: 31-32, 47-48, 63-64
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@myc.json` around lines 15 - 16, The JSON timestamps use local offsets (e.g., "2026-04-11T15:49:00.295713817+02:00"); convert and normalize all timestamps for keys like "created_at" and "updated_at" to UTC Zulu format (ISO-8601 with trailing Z) so they become deterministic (adjust the time value to UTC and replace the offset with Z), and apply the same change for the other occurrences mentioned (the other created_at/updated_at entries in this file).
11-11: Normalizetagsto a single data type.Line 43 uses a comma-delimited string while Lines 11, 27, and 59 use
null. This mixed shape increases parser branching and weakens schema stability. Prefertagsasstring[](or consistentlynull/array).Suggested schema-oriented adjustment
- "tags": "Active Node Test Hardening,testing,typescript,maintenance,node,tokens", + "tags": ["Active Node Test Hardening", "testing", "typescript", "maintenance", "node", "tokens"],Also applies to: 27-27, 43-43, 59-59
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@myc.json` at line 11, Normalize the "tags" property to a consistent string[] shape across the JSON: replace any occurrences of "tags": null with an empty array "tags": [] and convert any comma-delimited tag string (e.g., "tag1, tag2") into an actual array of trimmed strings (["tag1","tag2"]); ensure every object uses the same type (string[]) so parsers can rely on a single shape and update any schema/consumers if they currently expect null.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bf44adb7-42f8-4d21-a1f2-83253b3d0573
📒 Files selected for processing (71)
.env.example.gitignoredocumentation/devs/upgradable-network-testing.mdmyc.jsonpackage.jsonplanning/adversarial_review/network_configs_editability_v2.mdplanning/adversarial_review/security_conditions_v2.mdplanning/adversarial_review/stackable_genesis_system_v2.mdplanning/adversarial_review/staking_research/00_research_inventory.mdplanning/adversarial_review/staking_research/01_lead_conclusions.mdplanning/adversarial_review/staking_research/02_sdk_gap_analysis.mdplanning/adversarial_review/staking_research/plan/plan.mdplanning/adversarial_review/staking_research/plan/spec.mdplanning/network_configs_editability.mdplanning/security_conditions.mdplanning/stackable_genesis_system.mdscripts/upgradable-network/cli.tsscripts/upgradable-network/e2e.shscripts/upgradable-network/gen-identity.tsscripts/upgradable-network/sdk-builders.test.tssrc/config/defaults.tssrc/config/envKeys.tssrc/config/loader.tssrc/config/types.tssrc/features/networkUpgrade/constants.tssrc/features/networkUpgrade/safetyBounds.tssrc/features/networkUpgrade/types.tssrc/features/staking/constants.tssrc/features/staking/types.tssrc/index.tssrc/libs/blockchain/chainBlocks.tssrc/libs/blockchain/gcr/gcr.tssrc/libs/blockchain/gcr/gcr_routines/GCRNetworkUpgradeRoutines.tssrc/libs/blockchain/gcr/gcr_routines/GCRValidatorStakeRoutines.tssrc/libs/blockchain/gcr/handleGCR.tssrc/libs/blockchain/routines/applyNetworkUpgrade.tssrc/libs/blockchain/routines/calculateCurrentGas.tssrc/libs/blockchain/routines/loadNetworkParameters.tssrc/libs/blockchain/routines/tallyUpgradeVotes.tssrc/libs/blockchain/routines/validateTransaction.tssrc/libs/blockchain/routines/validatorsManagement.tssrc/libs/network/dtr/dtrmanager.tssrc/libs/network/endpointExecution.tssrc/libs/network/handlers/governanceHandlers.tssrc/libs/network/handlers/index.tssrc/libs/network/handlers/validatorHandlers.tssrc/libs/network/routines/transactions/handleGovernanceTx.tssrc/libs/network/routines/transactions/handleStakingTx.tssrc/libs/utils/demostdlib/deriveMempoolOperation.tssrc/model/datasource.tssrc/model/entities/NetworkUpgrade.tssrc/model/entities/NetworkUpgradeVote.tssrc/model/entities/Validators.tssrc/types/demosdk-x-augmentations.d.tssrc/utilities/sharedState.tstesting/devnet/docker-compose.ymltests/governance/applyNetworkUpgrade.test.tstests/governance/concurrentProposals.test.tstests/governance/deriveFeesFromParameters.test.tstests/governance/e2e.test.tstests/governance/governanceHandlers.test.tstests/governance/handleGovernanceTx.test.tstests/governance/loadNetworkParameters.test.tstests/governance/safetyBounds.test.tstests/governance/snapshotWeightIntegrity.test.tstests/governance/tallyUpgradeVotes.test.tstests/staking/gcrValidatorStakeRoutines.test.tstests/staking/handleStakingTx.test.tstests/staking/integration.test.tstests/staking/validatorHandlers.test.tstests/staking/validatorsManagement.test.ts
…urrentBlock support and improved error handling
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/libs/blockchain/gcr/gcr_routines/GCRNetworkUpgradeRoutines.ts (2)
123-133:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing check for proposal status before recording vote.
The code validates that the proposal exists but doesn't verify it's still
"pending". Votes for proposals that have already been tallied (status:approved,rejected,activating,active) should be skipped to prevent late or invalid votes from contaminating the database.Proposed fix
const proposal = await proposals.findOneBy({ proposalId: e.proposalId, }) if (!proposal) { // Should have been rejected at RPC entry; skip silently here // so a leaked vote doesn't break block confirmation. return { success: true, message: `Vote skipped: proposal ${e.proposalId} not found`, } } + if (proposal.status !== "pending") { + return { + success: true, + message: `Vote skipped: proposal ${e.proposalId} is ${proposal.status}`, + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libs/blockchain/gcr/gcr_routines/GCRNetworkUpgradeRoutines.ts` around lines 123 - 133, The code currently returns early only when a proposal is not found; after retrieving the proposal via proposals.findOneBy (inside GCRNetworkUpgradeRoutines), add a check on proposal.status and skip recording the vote unless proposal.status === "pending" — for any other statuses ("approved","rejected","activating","active", etc.) return a success/skip response similar to the not-found branch with a message like `Vote skipped: proposal ${e.proposalId} status is ${proposal.status}` so late or invalid votes are ignored; place this check immediately after the proposal lookup and before any vote-recording logic.
134-134:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
blockNumberignores the injectedcurrentBlockparameter.The method signature accepts
currentBlock?: number(line 96), but line 134 unconditionally callsChain.getLastBlockNumber(). This can cause the recordedblockNumberto drift from the block being applied, especially when block N is being processed but the chain tip still reports N-1.Proposed fix
- const blockNumber = await Chain.getLastBlockNumber() + const blockNumber = currentBlock ?? (await Chain.getLastBlockNumber())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libs/blockchain/gcr/gcr_routines/GCRNetworkUpgradeRoutines.ts` at line 134, The code unconditionally assigns blockNumber from Chain.getLastBlockNumber() inside GCRNetworkUpgradeRoutines while the routine/method accepts currentBlock?: number; change the blockNumber assignment to use the injected currentBlock when provided (e.g., const blockNumber = currentBlock ?? await Chain.getLastBlockNumber()) so the routine records the block being applied instead of always using the chain tip; update any related references in the same method (blockNumber variable) to ensure they use this corrected value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/libs/blockchain/gcr/gcr_routines/GCRNetworkUpgradeRoutines.ts`:
- Around line 123-133: The code currently returns early only when a proposal is
not found; after retrieving the proposal via proposals.findOneBy (inside
GCRNetworkUpgradeRoutines), add a check on proposal.status and skip recording
the vote unless proposal.status === "pending" — for any other statuses
("approved","rejected","activating","active", etc.) return a success/skip
response similar to the not-found branch with a message like `Vote skipped:
proposal ${e.proposalId} status is ${proposal.status}` so late or invalid votes
are ignored; place this check immediately after the proposal lookup and before
any vote-recording logic.
- Line 134: The code unconditionally assigns blockNumber from
Chain.getLastBlockNumber() inside GCRNetworkUpgradeRoutines while the
routine/method accepts currentBlock?: number; change the blockNumber assignment
to use the injected currentBlock when provided (e.g., const blockNumber =
currentBlock ?? await Chain.getLastBlockNumber()) so the routine records the
block being applied instead of always using the chain tip; update any related
references in the same method (blockNumber variable) to ensure they use this
corrected value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d8a5c323-d686-41ea-bf4c-3479aa14402d
📒 Files selected for processing (2)
src/libs/blockchain/gcr/gcr_routines/GCRNetworkUpgradeRoutines.tssrc/libs/blockchain/gcr/gcr_routines/GCRValidatorStakeRoutines.ts
…nd error handling
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (6)
tests/staking/gcrValidatorStakeRoutines.test.ts (2)
106-109: ⚡ Quick winAdd one test for the
currentBlockfallback path.On Line 108,
Chain.getLastBlockNumberis mocked, but everyapply(...)call in this suite passescurrentBlock, so the fallback branch is currently untested.Suggested test addition
describe("GCRValidatorStakeRoutines", () => { beforeEach(() => { jest.clearAllMocks() ;(Chain.getLastBlockNumber as jest.Mock).mockResolvedValue(100 as never) }) + it("uses Chain.getLastBlockNumber when currentBlock is omitted", async () => { + const existing = validatorRow() + const repo = createMockRepo(existing) + const r = await GCRValidatorStakeRoutines.apply( + stakeEdit({ operation: "unstake", amount: "0" }) as any, + repo as any, + ) + expect(r.success).toBe(true) + expect(Chain.getLastBlockNumber).toHaveBeenCalledTimes(1) + expect(repo.state.row?.unstake_requested_at).toBe(100) + expect(repo.state.row?.unstake_available_at).toBe(100 + UNSTAKE_LOCK_BLOCKS) + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/staking/gcrValidatorStakeRoutines.test.ts` around lines 106 - 109, Add a unit test that exercises the "currentBlock" fallback by forcing Chain.getLastBlockNumber to fail or return undefined and then calling the same apply(...) flow that supplies a currentBlock value; verify apply(...) used that supplied currentBlock (and any downstream behavior/assertions expected). Specifically, mock (Chain.getLastBlockNumber as jest.Mock) to reject or resolve to undefined, call the existing apply(...) invocation that passes the currentBlock argument, and assert the code path that relies on the supplied currentBlock executed (e.g., expectations on apply, returned values, or side-effects).
1-8: Align Jest package majors to avoid subtle mock/type drift.The test file relies on
@jest/globals, andpackage.jsonspecifies@jest/globals@^30.2.0alongsidejest@^29.7.0. Keeping them on the same major version ensures consistency in runtime behavior and type definitions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/staking/gcrValidatorStakeRoutines.test.ts` around lines 1 - 8, The test imports Jest globals from "@jest/globals" while package.json pins "jest" to major 29, causing potential runtime/type mismatches; pick one major and align both packages (either upgrade "jest" to a 30.x major to match "@jest/globals@^30.2.0" or downgrade "@jest/globals" to a 29.x release), update package.json accordingly, reinstall / regenerate lockfile, and run the test suite to confirm the globals import (the beforeAll/beforeEach/describe/it/expect/jest imports) behaves correctly.src/libs/blockchain/routines/loadNetworkParameters.ts (1)
58-58: 💤 Low valueRemove unnecessary leading semicolons.
ESLint correctly flags the semicolons at lines 58, 65, 66, and 68 as unnecessary — they follow closing braces where ASI hazards don't apply.
🧹 Proposed cleanup
} else { - ;(params as unknown as Record<string, unknown>)[key] = value + (params as unknown as Record<string, unknown>)[key] = value } } } getSharedState.networkParameters = params // Mirror onto flat fields read by calculateCurrentGas / getShard. - ;(getSharedState as unknown as { rpcFee: number }).rpcFee = params.rpcFee - ;(getSharedState as unknown as { networkFee: number }).networkFee = + (getSharedState as unknown as { rpcFee: number }).rpcFee = params.rpcFee + (getSharedState as unknown as { networkFee: number }).networkFee = params.networkFee - ;(getSharedState as unknown as { shardSize: number }).shardSize = + (getSharedState as unknown as { shardSize: number }).shardSize = params.shardSize🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libs/blockchain/routines/loadNetworkParameters.ts` at line 58, In function loadNetworkParameters remove the unnecessary leading semicolons before statements (e.g., the line that does (params as unknown as Record<string, unknown>)[key] = value and the other following-brace statements in the same function); simply delete the extra semicolons so the assignments/expressions start directly with the expression, leaving the rest of the code unchanged.documentation/devs/upgradable-network-testing.md (1)
133-154: 💤 Low valueAdd language specifier to fenced code block.
The directory tree code block at line 133 is missing a language identifier. Adding
textorplaintextsatisfies markdownlint and improves consistency.📝 Suggested fix
-``` +```text tests/ ├── governance/ # Phase 1 unit suites (10 files)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@documentation/devs/upgradable-network-testing.md` around lines 133 - 154, Update the fenced code block that starts with the directory tree (the block containing "tests/", "scripts/upgradable-network/", "testing/devnet/", "e2e-runs/") in documentation/devs/upgradable-network-testing.md to include a language specifier (e.g., change the opening ``` to ```text or ```plaintext) so markdownlint passes and the block is consistently rendered.src/libs/network/handlers/governanceHandlers.ts (1)
200-212: 💤 Low valueComment contradicts underlying behavior.
Lines 203-204 state "Throws on validator-set lookup failure," but
GCR.getGCRValidatorsAtBlock()catches exceptions internally and returns[](persrc/libs/blockchain/gcr/gcr.ts:340-368). A snapshot lookup failure silently producestotal = 0n, which could incorrectly reject all proposals rather than surfacing an error.Consider either:
- Updating the comment to reflect the actual fallback-to-empty behavior, or
- Checking if
validators.length === 0and logging a warning when a non-zero snapshot is expected🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libs/network/handlers/governanceHandlers.ts` around lines 200 - 212, The comment on computeSnapshotWeight incorrectly claims GCR.getGCRValidatorsAtBlock throws on failure while it actually returns [] on error; update computeSnapshotWeight to detect an empty validators array (if validators.length === 0) and either (a) throw a descriptive error referencing the snapshotBlock so callers can surface the failure, or (b) if you prefer to keep the fallback, change the comment to state that the function falls back to an empty validator set and optionally log a warning when a non-zero snapshot was expected; locate this logic in computeSnapshotWeight and the call to GCR.getGCRValidatorsAtBlock to implement the chosen fix.scripts/upgradable-network/e2e.sh (1)
78-85: 💤 Low valueMisleading
require psqlcheck —psql_nuses Docker, not local psql.Line 80 checks for local
psqlwith|| true(non-fatal), but thepsql_nfunction at Line 104 executesdocker exec ... psqlinside the container. The local psql check is unnecessary and could confuse maintainers about dependencies.Consider removing the
require psql || truecheck since it's not actually used.Proposed fix
-require docker; require curl; require jq; require bunx; require psql || true +require docker; require curl; require jq; require bunx🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/upgradable-network/e2e.sh` around lines 78 - 85, The script currently runs a misleading local psql check (`require psql || true`) even though the helper psql_n() invokes psql inside a container via docker exec; remove the `require psql || true` invocation from the preflight block (or replace it with an explicit check that Docker is available if you want an explicit container runtime dependency) so that only the actual dependency check (e.g., require docker) remains; update any comments referencing local psql to reference the containerized psql usage in psql_n().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@documentation/devs/upgradable-network-testing.md`:
- Around line 133-154: Update the fenced code block that starts with the
directory tree (the block containing "tests/", "scripts/upgradable-network/",
"testing/devnet/", "e2e-runs/") in
documentation/devs/upgradable-network-testing.md to include a language specifier
(e.g., change the opening ``` to ```text or ```plaintext) so markdownlint passes
and the block is consistently rendered.
In `@scripts/upgradable-network/e2e.sh`:
- Around line 78-85: The script currently runs a misleading local psql check
(`require psql || true`) even though the helper psql_n() invokes psql inside a
container via docker exec; remove the `require psql || true` invocation from the
preflight block (or replace it with an explicit check that Docker is available
if you want an explicit container runtime dependency) so that only the actual
dependency check (e.g., require docker) remains; update any comments referencing
local psql to reference the containerized psql usage in psql_n().
In `@src/libs/blockchain/routines/loadNetworkParameters.ts`:
- Line 58: In function loadNetworkParameters remove the unnecessary leading
semicolons before statements (e.g., the line that does (params as unknown as
Record<string, unknown>)[key] = value and the other following-brace statements
in the same function); simply delete the extra semicolons so the
assignments/expressions start directly with the expression, leaving the rest of
the code unchanged.
In `@src/libs/network/handlers/governanceHandlers.ts`:
- Around line 200-212: The comment on computeSnapshotWeight incorrectly claims
GCR.getGCRValidatorsAtBlock throws on failure while it actually returns [] on
error; update computeSnapshotWeight to detect an empty validators array (if
validators.length === 0) and either (a) throw a descriptive error referencing
the snapshotBlock so callers can surface the failure, or (b) if you prefer to
keep the fallback, change the comment to state that the function falls back to
an empty validator set and optionally log a warning when a non-zero snapshot was
expected; locate this logic in computeSnapshotWeight and the call to
GCR.getGCRValidatorsAtBlock to implement the chosen fix.
In `@tests/staking/gcrValidatorStakeRoutines.test.ts`:
- Around line 106-109: Add a unit test that exercises the "currentBlock"
fallback by forcing Chain.getLastBlockNumber to fail or return undefined and
then calling the same apply(...) flow that supplies a currentBlock value; verify
apply(...) used that supplied currentBlock (and any downstream
behavior/assertions expected). Specifically, mock (Chain.getLastBlockNumber as
jest.Mock) to reject or resolve to undefined, call the existing apply(...)
invocation that passes the currentBlock argument, and assert the code path that
relies on the supplied currentBlock executed (e.g., expectations on apply,
returned values, or side-effects).
- Around line 1-8: The test imports Jest globals from "@jest/globals" while
package.json pins "jest" to major 29, causing potential runtime/type mismatches;
pick one major and align both packages (either upgrade "jest" to a 30.x major to
match "@jest/globals@^30.2.0" or downgrade "@jest/globals" to a 29.x release),
update package.json accordingly, reinstall / regenerate lockfile, and run the
test suite to confirm the globals import (the
beforeAll/beforeEach/describe/it/expect/jest imports) behaves correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0a1fc6bb-dd8c-4f74-87e1-333e54652bb8
📒 Files selected for processing (14)
documentation/devs/upgradable-network-testing.mdscripts/upgradable-network/cli.tsscripts/upgradable-network/e2e.shsrc/features/networkUpgrade/constants.tssrc/features/networkUpgrade/safetyBounds.tssrc/libs/blockchain/gcr/gcr_routines/GCRNetworkUpgradeRoutines.tssrc/libs/blockchain/routines/applyNetworkUpgrade.tssrc/libs/blockchain/routines/loadNetworkParameters.tssrc/libs/blockchain/routines/tallyUpgradeVotes.tssrc/libs/network/handlers/governanceHandlers.tssrc/libs/network/routines/transactions/handleGovernanceTx.tssrc/libs/utils/demostdlib/deriveMempoolOperation.tstests/governance/safetyBounds.test.tstests/staking/gcrValidatorStakeRoutines.test.ts
✅ Files skipped from review due to trivial changes (2)
- src/libs/blockchain/routines/applyNetworkUpgrade.ts
- tests/governance/safetyBounds.test.ts
…nto upgradable_network
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/libs/blockchain/gcr/handleGCR.ts (1)
1118-1140: 💤 Low valueConsider extracting duplicated dynamic import.
The dynamic import of
GCRNetworkUpgradeRoutinesis duplicated between thenetworkUpgradeandnetworkUpgradeVotecases. This adds latency on each path and could be extracted.Also note the inconsistency:
GCRValidatorStakeRoutinesis statically imported (line 58) whileGCRNetworkUpgradeRoutinesuses dynamic import. If code splitting is intentional for governance, the duplicate import could at least be hoisted:♻️ Suggested refactor to deduplicate dynamic import
case "networkUpgrade": if (simulate) { result = { success: true, message: "Simulated" } } else { - const { default: GCRNetworkUpgradeRoutines } = await import( - "./gcr_routines/GCRNetworkUpgradeRoutines" - ) + const GCRNetworkUpgradeRoutines = (await import( + "./gcr_routines/GCRNetworkUpgradeRoutines" + )).default result = await GCRNetworkUpgradeRoutines.applyProposal( editOperation, ) } break case "networkUpgradeVote": if (simulate) { result = { success: true, message: "Simulated" } } else { - const { default: GCRNetworkUpgradeRoutines } = await import( - "./gcr_routines/GCRNetworkUpgradeRoutines" - ) + const GCRNetworkUpgradeRoutines = (await import( + "./gcr_routines/GCRNetworkUpgradeRoutines" + )).default result = await GCRNetworkUpgradeRoutines.applyVote( editOperation, ) } breakOr for consistency with
GCRValidatorStakeRoutines, convert to a static import at the top of the file if code splitting isn't a requirement.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libs/blockchain/gcr/handleGCR.ts` around lines 1118 - 1140, The dynamic import of GCRNetworkUpgradeRoutines is duplicated in the networkUpgrade and networkUpgradeVote cases; extract and reuse a single import before the switch or at the top of the function so both branches call the same loaded module; specifically, load GCRNetworkUpgradeRoutines once (either via a single await import("./gcr_routines/GCRNetworkUpgradeRoutines") stored in a const) and then call its applyProposal(editOperation) in the networkUpgrade case and applyVote(editOperation) in the networkUpgradeVote case (keeping the existing simulate checks), or alternatively convert GCRNetworkUpgradeRoutines to a static import like GCRValidatorStakeRoutines if code splitting is not required.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/libs/blockchain/gcr/gcr.ts`:
- Around line 322-325: When accumulating stakes into total, reject negative
values instead of allowing BigInt(raw) to decrease the sum: for each v (the
stake object) compute raw = v.staked_amount ?? "0", detect if raw represents a
negative value (e.g., starts with '-' or parsed BigInt < 0) and skip that entry
(or treat it as "0") before doing total += BigInt(raw); update the accumulation
block around total, raw, and BigInt(raw) so negatives are not summed and
optionally log or count skipped negatives for visibility.
---
Nitpick comments:
In `@src/libs/blockchain/gcr/handleGCR.ts`:
- Around line 1118-1140: The dynamic import of GCRNetworkUpgradeRoutines is
duplicated in the networkUpgrade and networkUpgradeVote cases; extract and reuse
a single import before the switch or at the top of the function so both branches
call the same loaded module; specifically, load GCRNetworkUpgradeRoutines once
(either via a single await import("./gcr_routines/GCRNetworkUpgradeRoutines")
stored in a const) and then call its applyProposal(editOperation) in the
networkUpgrade case and applyVote(editOperation) in the networkUpgradeVote case
(keeping the existing simulate checks), or alternatively convert
GCRNetworkUpgradeRoutines to a static import like GCRValidatorStakeRoutines if
code splitting is not required.
🪄 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: 9faeb85e-4ede-4352-ba48-915a4d63c4bd
📒 Files selected for processing (8)
.env.example.gitignorepackage.jsonsrc/index.tssrc/libs/blockchain/gcr/gcr.tssrc/libs/blockchain/gcr/handleGCR.tssrc/libs/network/endpointExecution.tssrc/utilities/sharedState.ts
✅ Files skipped from review due to trivial changes (2)
- .gitignore
- package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- src/index.ts
Greptile SummaryThis PR introduces on-chain governance (propose/vote/tally/activate network parameter upgrades) and a Phase-0 staking lifecycle (stake/unstake/exit) built on top of the existing GCR system. It adds two new TypeORM entities, a block-level hook in
Confidence Score: 3/5Multiple correctness gaps across the governance and staking paths need resolution before this lands on a live chain. The GCRNetworkUpgradeRoutines.ts (effectiveAtBlock grace-period re-validation), Validators.ts (column rename migration), handleGCR.ts (transactional EM gap), tallyUpgradeVotes.ts (exact-equality query) Important Files Changed
Sequence DiagramsequenceDiagram
participant Client
participant RPC as confirmTransaction
participant Dispatcher as runTypeDispatcher
participant Mempool
participant Block as insertBlock
participant GCR as applyGCREdit
participant Tally as tallyUpgradeVotes
participant Apply as applyNetworkUpgrade
participant State as loadNetworkParameters
Client->>RPC: submit networkUpgrade tx
RPC->>Dispatcher: handleGovernanceTx (validation only)
Dispatcher-->>RPC: ok=true
RPC-->>Client: signed validityData
Client->>Mempool: addTransaction
Note over Block: At block N
Block->>GCR: applyGCREdit networkUpgrade
GCR->>GCR: applyProposal stores row with snapshotBlock=N
Block->>Tally: tallyUpgradeVotes(N)
Tally-->>Block: pending to activating or rejected
Block->>Apply: applyNetworkUpgrade(N)
Apply-->>Block: ActivationOutcome list
Block-->>State: loadNetworkParameters post-commit
Reviews (5): Last reviewed commit: "Merge branch 'stabilisation' into upgrad..." | Re-trigger Greptile |
…rfile compatibility
…` mocks and other no-extra-semi violations Auto-applied by `bun run lint:fix` across files not otherwise touched by the PR #778 review fixes (Mycelium epic E#6). Pure cosmetic: leading semicolons before `(...)` expressions are removed in favor of `jest.mocked(...)` and other ASI-safe forms. No behavior change.
…ock in applyProposal PR #778 review G-6: handleGovernanceTx enforces `effectiveAtBlock >= chainTip + VOTING_WINDOW + GRACE_PERIOD` at RPC submission, but a tx that sits in the mempool can have its chain-tip-relative invariant broken by the time the block is confirmed. Without re-checking against the freshly-computed snapshotBlock, an upgrade can land with zero grace instead of the required 50 blocks. Adds `minEffectiveAtBlock = tallyBlock + GRACE_PERIOD_BLOCKS` floor in applyProposal and rejects the proposal when the field comes in below. Mycelium task #61.
…Int / computeSnapshotWeight kernel PR #778 review: - G-5: exact-equality `tallyBlock: currentBlock` lookup would leave a proposal stuck in "pending" forever if its tally block was ever skipped (devnet reset, reorg). Switch to `LessThanOrEqual` so overdue proposals are tallied on the next post-block pass. `applyNetworkUpgrade.ts` already uses a JS-side `<=` filter and is unaffected. - G-2: `safeBigInt` was duplicated identically in tallyUpgradeVotes and governanceHandlers; `computeSnapshotWeight` was duplicated with deliberately divergent error policies. Extract to `src/features/networkUpgrade/governanceWeight.ts` — shared `safeBigInt` and a re-throwing `computeSnapshotWeight` kernel that each caller wraps with its own policy (tally returns 0n on lookup failure for determinism; RPC handler surfaces the error). Mycelium tasks #63 and #65.
PR #778 review G-3: when current=0, the percent-based cap is undefined (delta/0). Previous behavior froze the parameter at 0 because `return proposed === 0n` permitted no movement. An operator who set NETWORK_FEE=0 via env could then never raise the parameter through governance. Allow any non-negative proposed value when absCurrent=0; absolute floor/ceiling per key still apply at the per-key check sites. Mycelium task #64.
PR #778 review G-1 + G-4: three independent `requireSender` helpers had diverged — handleGovernanceTx lowercased, handleStakingTx and validatorsManagement did not. A validator registered under a mixed-case 0x address therefore had every governance vote and proposal silently rejected, because the lower-cased sender used in lookups never matched the DB row stored verbatim. Single canonical helper in `src/libs/network/utils/txHelpers.ts`: - `requireSender(tx)` — always returns the lower-cased form - `canonicalAddress(addr)` — same rule, used at every persistence boundary so producer (staking insert) and consumer (governance lookup) sides agree Apply at insert sites: - `GCRValidatorStakeRoutines.apply` lowercases `account` before any DB key/find/save - `GCRNetworkUpgradeRoutines.applyProposal` stores `proposerPublicKey` lowercased - `GCRNetworkUpgradeRoutines.applyVote` lowercases `voterAddress` and matches the snapshot validator set against the canonical form Address case is purely a display concern (Ethereum-style checksum, ed25519 hex). Storing the canonical lowercase form once removes a whole class of comparison-mismatch bugs. Mycelium task #60.
PR #778 review M-2: the gas formula change in this branch silently doubled the flat-fee component (rpcFee + networkFee = 10 + 10 = 20) versus pre-PR (rpcFee = 10). Wallets and SDKs hardcoding a flat-fee budget would start hitting "insufficient gas" with no signal that the formula changed. Reshape to the explicit three-component model: total tx cost = networkFee + rpcFee + burnFee defaults = 1 + 1 + 1 = 3 - Adds `burnFee` as a node-local config knob (env BURN_FEE; mirrored onto sharedState). Once the SDK adds `burnFee` to NetworkParameters it can be folded into governance like the other two — comments in place at every relevant site. - Drops `adaptedGas` from the per-tx cost. Replaced with a `dynamicSurgeMultiplier()` stub that returns 1; the seam is wired so re-enabling congestion pricing later is a one-function change. - Lowers genesis defaults from 10/10 to 1/1 in HARDCODED_FALLBACK_NETWORK_PARAMETERS to match. - TODO(decimals): once OS denomination ships (Mycelium E#3), the three components must total exactly 1 DEM (≈ 333_333_333 OS each, exact split TBD). Comments planted at every site. Test fixtures that proposed networkFee deltas against the genesis default (e.g. 1 → 15) now stage `networkFee=10` in sharedState before running so the 50% percent cap leaves room for the proposal — the behavior under test is governance flow, not the genesis defaults. Mycelium task #62.
PR #778 review CR-5: the bot's original concern (only EXITED rejected, all other statuses pass) was already fixed — current code requires `status === ACTIVE || status === UNSTAKING`. Add explicit tests for the rejection paths (null, undefined, unknown string) so the allow-list stays under regression protection. Mycelium task #66.
|
35 tasks
tcsenpai
added a commit
that referenced
this pull request
May 12, 2026
…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).
tcsenpai
added a commit
that referenced
this pull request
May 12, 2026
…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.
6 tasks
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
3 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 by CodeRabbit
New Features
Chores
Documentation
Tests