jsonrpc: Add generic-based dispatch for RPC handler calls#3642
Conversation
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (35.45%) is below the target coverage (60.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## try/sonic-json #3642 +/- ##
==================================================
- Coverage 76.00% 74.70% -1.30%
==================================================
Files 388 392 +4
Lines 35000 34489 -511
==================================================
- Hits 26600 25766 -834
- Misses 6530 6841 +311
- Partials 1870 1882 +12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR refactors JSON-RPC method registration/dispatch to use generic registration helpers that build typed per-method Dispatch closures (instead of reflecting on handler signatures at request time). It updates RPC v8/v9/v10 to expose RegisterMethods() (plus param-shape structs) and adapts server/tests to the new jsonrpc.Method shape.
Changes:
- Introduces
jsonrpc.Register/RegisterC/RegisterH/RegisterCHto generate typedDispatchhandlers and param decoding/validation logic. - Migrates RPC v8/v9/v10 method lists to
Handler.RegisterMethods()+ param structs, and simplifiesrpc.Handlerversion method assembly. - Updates JSON-RPC server/tests/benchmarks to the new
MethodAPI (Dispatch + ParamStructType) and removes old reflection-based invocation.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| rpc/v9/subscriptions_test.go | Switches tests to register v9 methods via handler.RegisterMethods(). |
| rpc/v9/register.go | Adds v9 method registrations using generic jsonrpc registration helpers. |
| rpc/v9/params.go | Adds v9 param structs used by generic registration/decoding. |
| rpc/v9/handlers.go | Removes legacy methods() used only for tests. |
| rpc/v8/subscriptions_test.go | Switches tests to register v8 methods via handler.RegisterMethods(). |
| rpc/v8/register.go | Adds v8 method registrations using generic jsonrpc registration helpers. |
| rpc/v8/params.go | Adds v8 param structs used by generic registration/decoding. |
| rpc/v8/handlers.go | Removes legacy methods() used only for tests. |
| rpc/v10/register.go | Adds v10 method registrations using generic jsonrpc registration helpers. |
| rpc/v10/params.go | Adds v10 param structs used by generic registration/decoding. |
| rpc/handlers.go | Rebuilds versioned method lists by composing per-version RegisterMethods() + juno_version. |
| rpc/handlers_test.go | Updates validator-compatibility tests to inspect ParamStructType instead of handler signatures. |
| jsonrpc/server.go | Replaces reflection-based handler invocation with Method.Dispatch and pretouch via constructor-provided type lists. |
| jsonrpc/register.go | Adds generic registration + param decoding logic (positional/named, optional via omitempty). |
| jsonrpc/register_internal_test.go | Adds internal unit tests for the new registration/decoding pipeline. |
| jsonrpc/server_test.go | Updates server tests to use generic registration and adjusts expected validator error paths. |
| jsonrpc/http_test.go | Updates HTTP tests to use generic registration with param structs. |
| jsonrpc/websocket_test.go | Updates websocket tests to use generic registration with param structs. |
| jsonrpc/server_bench_test.go | Updates hot-path benchmarks to use generic registration with param structs. |
| jsonrpc/server_internal_test.go | Removes tests for now-deleted reflection helper logic. |
Comments suppressed due to low confidence (3)
rpc/v10/params.go:35
- Same issue here:
response_flagsis intended to be optional (missing should mean default flags). Withoutomitempty, the new param decoder will treat it as required and return Invalid Params for callers that omit it.
type TxHashFlagsParams struct {
TransactionHash *felt.Felt `json:"transaction_hash"`
ResponseFlags ResponseFlags `json:"response_flags"`
}
rpc/v10/params.go:47
response_flagsforstarknet_getTransactionByBlockIdAndIndexwas previously optional; withoutomitemptyit becomes required under the new registration/decoding scheme, breaking backward compatibility. Make this field optional so omitted flags decode to the zero value.
type TxByBlockIDAndIndexParams struct {
BlockID *BlockID `json:"block_id"`
Index int `json:"index"`
ResponseFlags ResponseFlags `json:"response_flags"`
}
rpc/v10/params.go:64
response_flagsonstarknet_getStorageAtwas previously optional; withoutomitemptythis becomes required and existing requests that omit it will fail with Invalid Params. Mark it optional (e.g.json:"response_flags,omitempty"and/or pointer type) to preserve prior behavior.
type StorageAtParams struct {
ContractAddress *felt.Address `json:"contract_address"`
Key *felt.Felt `json:"key"`
BlockID *BlockID `json:"block_id"`
Flags StorageAtResponseFlags `json:"response_flags"`
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| name, opts, _ := strings.Cut(tag, ",") | ||
| optional := opts == "omitempty" | ||
| plan.byName[name] = len(plan.tags) | ||
| plan.tags = append(plan.tags, name) | ||
| plan.optional = append(plan.optional, optional) | ||
| if !optional { | ||
| plan.requiredN++ | ||
| } |
| func (h *Handler) RegisterMethods() []jsonrpc.Method { | ||
| return []jsonrpc.Method{ | ||
| jsonrpc.Register("starknet_chainId", | ||
| func(_ *jsonrpc.NoParams) (*felt.Felt, *jsonrpc.Error) { return h.ChainID() }), |
There was a problem hiding this comment.
We wrap handler calls in closures to pass the args. Potentially, we could drop it, and pass handlers directly, but we'd need to update their signatures. I've decided not to, as the PR is big enough already, and if needed, it could be done in the follow-up.
| // on several handlers (BlockID, felt.Felt) so the param fields use | ||
| // value types where the underlying handler does. | ||
|
|
||
| type BlockIDParams struct { |
There was a problem hiding this comment.
There are some inconsistencies in v8 handler params. I've decided to match these, rather than refactor/update the handler's signatures.
|
@brbrr can you do a comparison against |
|
@rodrodros bench comparison is against sonic branch. I've updated the description. |
Note: this PR targets
try/sonic-json, not themainbranchBench comparison against try/sonic-json branch:
no_paramsone_scalar_posone_struct_posthree_pos_mixedthree_named_requirednamed_opt_missingnamed_opt_presentvalidator_passvalidator_failtype_errorbatch_small(5 reqs)batch_large(50 reqs)response_100_structs