Added RPC Parameters endpoint#779
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a new RPC method "parameters" implemented in bin/rpc/src/methods/parameters.ts: a Zod-validated V1 schema and an exported async function that returns a structured object populated from chainSpec fields and project constants. Wires the new method into the RPC registry by importing it in bin/rpc/src/method-loader.ts and replacing the previous notImplemented placeholder. Also changes two constants in packages/jam/block/work-item-segment.ts (W_E and W_S) from internal const to exported const. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
DrEverr
requested review from
skoszuta and
tomusdrw
and removed request for
tomusdrw
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
bin/rpc/src/method-loader.ts(2 hunks)bin/rpc/src/methods/parameters.ts(1 hunks)packages/jam/block/work-item-segment.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.ts
⚙️ CodeRabbit configuration file
**/*.ts: rules from./CODESTYLE.mdshould be adhered to.
**/*.ts: Any function whose documentation mention it must not be used in production code,
can be safely used in*.test.tsfiles. Other usage should be carefuly reviewed
and the comment must explain why it's safe to use there.
**/*.ts:asconversions must not be used. Suggest usingtryAsconversion methods.
**/*.ts: Classes withstatic Codecfield must have private constructor and staticcreatemethod.
**/*.ts: Casting abigint(orU64) usingNumber(x)must have an explanation comment why
it is safe.
**/*.ts: When making changes to code with comments containing links (in classes, constants, methods, etc.)
to graypaper.fluffylabs.dev, ensure those links point to the current version for this update.
Files:
packages/jam/block/work-item-segment.tsbin/rpc/src/methods/parameters.tsbin/rpc/src/method-loader.ts
🧠 Learnings (1)
📚 Learning: 2025-05-22T18:08:38.725Z
Learnt from: tomusdrw
Repo: FluffyLabs/typeberry PR: 381
File: bin/rpc/src/server.ts:99-112
Timestamp: 2025-05-22T18:08:38.725Z
Learning: RPC methods should implement strict parameter validation, including checking the exact number of parameters and their types. This helps catch errors early when users confuse similar methods (e.g., serviceData vs serviceValue) and prevents cryptic errors from undefined parameters being passed to internal code.
Applied to files:
bin/rpc/src/methods/parameters.ts
🧬 Code graph analysis (2)
bin/rpc/src/methods/parameters.ts (7)
bin/rpc/src/types.ts (2)
withValidation(84-92)NoArgs(114-114)packages/jam/state/service.ts (3)
BASE_SERVICE_BALANCE(23-23)ELECTIVE_ITEM_BALANCE(29-29)ELECTIVE_BYTE_BALANCE(35-35)packages/jam/transition/reports/verify-post-signature.ts (1)
G_A(8-8)packages/jam/block/gp-constants.ts (7)
G_I(18-18)O(24-24)Q(27-27)T(33-33)W_B(39-39)W_C(42-42)W_T(51-51)packages/jam/block/work-package.ts (1)
MAX_NUMBER_OF_WORK_ITEMS(28-28)packages/jam/transition/assurances.ts (1)
REPORT_TIMEOUT_GRACE_PERIOD(53-53)packages/jam/block/work-item-segment.ts (1)
W_E(6-6)
bin/rpc/src/method-loader.ts (1)
bin/rpc/src/methods/parameters.ts (1)
parameters(63-105)
tomusdrw
left a comment
tomusdrw
left a comment
There was a problem hiding this comment.
hackmd docs should be considered outdated now. Please reference https://github.com/polkadot-fellows/JIPs/blob/main/JIP-2.md#chain-parameters
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
bin/rpc/src/methods/parameters.ts (2)
65-67: Add safety comments for BigInt-to-number conversions.As noted in the previous review and per coding guidelines, casting
bigintusingNumber(x)requires an explanation comment. While these specific values (100n, 10n, 1n) are clearly safe, the guideline mandates documentation.Apply this diff:
- deposit_per_account: Number(BASE_SERVICE_BALANCE), - deposit_per_item: Number(ELECTIVE_ITEM_BALANCE), - deposit_per_byte: Number(ELECTIVE_BYTE_BALANCE), + deposit_per_account: Number(BASE_SERVICE_BALANCE), // safe: 100n < Number.MAX_SAFE_INTEGER + deposit_per_item: Number(ELECTIVE_ITEM_BALANCE), // safe: 10n < Number.MAX_SAFE_INTEGER + deposit_per_byte: Number(ELECTIVE_BYTE_BALANCE), // safe: 1n < Number.MAX_SAFE_INTEGERAs per coding guidelines
72-73: Add safety comments for BigInt conversions to Number.Lines 72-73 convert
U64values (which arebiginttype) tonumberwithout safety documentation. Per coding guidelines, such conversions must include comments explaining why the range is safe.Since
maxRefineGasandmaxBlockGasare constrained by chain configuration to small values (e.g., tinyChainSpec uses 1_000_000_000 and 20_000_000 respectively), these are safely withinNumber.MAX_SAFE_INTEGER. Add clarifying comments:- max_refine_gas: Number(chainSpec.maxRefineGas), - block_gas_limit: Number(chainSpec.maxBlockGas), + max_refine_gas: Number(chainSpec.maxRefineGas), // safe: chain config constrains to < Number.MAX_SAFE_INTEGER + block_gas_limit: Number(chainSpec.maxBlockGas), // safe: chain config constrains to < Number.MAX_SAFE_INTEGERAlso apply the same comment pattern to lines 65-67 for consistency (BASE_SERVICE_BALANCE, ELECTIVE_ITEM_BALANCE, ELECTIVE_BYTE_BALANCE are also bigint values being converted).
🧹 Nitpick comments (2)
bin/rpc/src/methods/parameters.ts (2)
15-56: Consider adding integer constraints to appropriate fields.Many of these parameters represent counts, periods, or sizes that should be integers (e.g.,
epoch_period,max_work_items,val_count). Consider using.int()refinements on those fields to enforce stricter validation at the API boundary.Example:
const V1 = z.object({ V1: z.object({ deposit_per_account: z.number(), deposit_per_item: z.number(), deposit_per_byte: z.number(), min_turnaround_period: z.number(), - epoch_period: z.number(), + epoch_period: z.number().int(), max_accumulate_gas: z.number(), max_is_authorized_gas: z.number(), - max_refine_gas: z.number(), + max_refine_gas: z.number().int(), // ... and so on for other integer fields }), });
68-68: Document or extract magic numbers to named constants.Several hardcoded numeric literals appear without explanation:
- Line 68:
32(min_turnaround_period)- Lines 89-93:
3072,64000,4096(max_imports, max_is_authorized_code_size, memory limits)- Line 95:
49152(max_report_elective_data)- Lines 100-101:
64000,4000000(max_authorizer_code_size, max_service_code_size)These should either reference named constants from the appropriate modules (like the other fields do) or include comments explaining their source and rationale.
Example:
- min_turnaround_period: 32, + min_turnaround_period: 32, // JIP-2 spec: minimum block interval for work package turnaroundOr extract to constants:
const MIN_TURNAROUND_PERIOD = 32; const MAX_IMPORTS = 3072; const MAX_AUTHORIZER_CODE_SIZE = 64000; // ... etc.Also applies to: 89-93, 95-95, 100-101
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
bin/rpc/src/methods/parameters.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.ts
⚙️ CodeRabbit configuration file
**/*.ts: rules from./CODESTYLE.mdshould be adhered to.
**/*.ts: Any function whose documentation mention it must not be used in production code,
can be safely used in*.test.tsfiles. Other usage should be carefuly reviewed
and the comment must explain why it's safe to use there.
**/*.ts:asconversions must not be used. Suggest usingtryAsconversion methods.
**/*.ts: Classes withstatic Codecfield must have private constructor and staticcreatemethod.
**/*.ts: Casting abigint(orU64) usingNumber(x)must have an explanation comment why
it is safe.
**/*.ts: When making changes to code with comments containing links (in classes, constants, methods, etc.)
to graypaper.fluffylabs.dev, ensure those links point to the current version for this update.
Files:
bin/rpc/src/methods/parameters.ts
🧠 Learnings (1)
📓 Common learnings
Learnt from: tomusdrw
Repo: FluffyLabs/typeberry PR: 381
File: bin/rpc/src/server.ts:99-112
Timestamp: 2025-05-22T18:08:38.725Z
Learning: RPC methods should implement strict parameter validation, including checking the exact number of parameters and their types. This helps catch errors early when users confuse similar methods (e.g., serviceData vs serviceValue) and prevents cryptic errors from undefined parameters being passed to internal code.
🧬 Code graph analysis (1)
bin/rpc/src/methods/parameters.ts (7)
bin/rpc/src/types.ts (2)
withValidation(84-92)NoArgs(114-114)packages/jam/state/service.ts (3)
BASE_SERVICE_BALANCE(23-23)ELECTIVE_ITEM_BALANCE(29-29)ELECTIVE_BYTE_BALANCE(35-35)packages/jam/transition/reports/verify-post-signature.ts (1)
G_A(8-8)packages/jam/block/gp-constants.ts (7)
G_I(18-18)O(24-24)Q(27-27)T(33-33)W_B(39-39)W_C(42-42)W_T(51-51)packages/jam/block/work-package.ts (1)
MAX_NUMBER_OF_WORK_ITEMS(28-28)packages/jam/transition/assurances.ts (1)
REPORT_TIMEOUT_GRACE_PERIOD(53-53)packages/jam/block/work-item-segment.ts (1)
W_E(6-6)
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: run (22.x)
- GitHub Check: run (22.x)
- GitHub Check: run (22.x)
- GitHub Check: run (22.x)
🔇 Additional comments (2)
bin/rpc/src/methods/parameters.ts (2)
1-13: LGTM: Imports are well-organized.All imported constants are properly utilized in the parameters function, and the import organization is clear and logical.
69-69: ChainSpec fields are guaranteed by TypeScript's type system—runtime validation is unnecessary.The function parameter
chainSpec: ChainSpec(bin/rpc/src/types.ts:87) is explicitly typed as non-optional. All accessed fields (epochLength, maxRefineGas, maxBlockGas, etc.) are directly assigned toz.number()properties in the return object. If any of these ChainSpec fields were optional in the type definition, accessing them would producenumber | undefined, causing TypeScript type errors when assigning to the schema'sz.number()properties. Since the code compiles without errors, the ChainSpec type guarantees all fields are non-optional. The concern about undefined values is already addressed by compile-time type checking.Likely an incorrect or invalid review comment.
View all
Benchmarks summary: 63/63 OK ✅ |
2 participants
https://hackmd.io/@polkadot/jip2#parameters
I've added a bit more fields than it its described in documentation.
I was trying to run Doom on our node and noticed that doom expects a bit more/different fields as well.
example of what jamtop, jamt, etc. expects at current stage
v 0.1.26