ChainSafe / ChainSafe/forest

[tracking] RPC refactoring wishlist

Open
#4,032 1 comment 0 reactions 0 assignees View on GitHub
Priority: 4 - Low Type: Enhancement
Dominant language
Rust
Stars
697
Forks
200
Avg merge
1d 5h
Merged PRs (30d)
65

Description

The motivating use-case is #4038.
While we're in the area we can do a lot of cleanup.

- #4067
- #4081
- #4007
- #4134
- #4172
- [x] Introduce `crate::rpc::Client`, remove `crate::rpc_client::ApiInfo`.
- #4140
- #4280
- #4231
- #4234
- [x] Migrate handlers to `trait RpcMethod`.
- Migration guide for RPC call sites is as follows. Deviations from this SHOULD be checked with me, and please add me as a reviewer to your PR
- Use the [snippets](https://github.com/ChainSafe/forest/issues/4032#issuecomment-2042626065) file to scaffold your implementation
- Make sure the method name matches!
- You may need to narrow the bounds on `Ctx`
- Ensure `rpc::types` are minimal - any structs used by only one e.g `gas` API should live in `gas.rs`, not in `types.rs`. `types.rs` is only for types that are shared across APIs
- Migrate the parameters, dropping `LotusJson` where possible.
- Implement `JsonSchema` where necessary.
- Add or amend the [`for_each_method`](https://github.com/ChainSafe/forest/blob/18c5f2617369942c5157223dc0bee5a7331a61e9/src/rpc/methods/auth.rs#L15-L21) macro in `gas.rs`.
- It should be called [to expose methods in the `prelude`](https://github.com/ChainSafe/forest/blob/18c5f2617369942c5157223dc0bee5a7331a61e9/src/rpc/mod.rs#L34)
- It should be called to [register methods](https://github.com/ChainSafe/forest/blob/18c5f2617369942c5157223dc0bee5a7331a61e9/src/rpc/mod.rs#L186)
- `RpcTest` constructors should use `RpcTest::basic_raw`, and `RpcMethodExt::request` [like this](https://github.com/ChainSafe/forest/blob/18c5f2617369942c5157223dc0bee5a7331a61e9/src/tool/subcommands/api_cmd.rs#L351C49-L351C57).
- `forest-cli` calls should `rpc::Client::from(api: ApiInfo)` and then `RpcMethodExt::call` on that.
In the limit, `ApiInfo` can be removed, and replaced by an `rpc::Client::from_env` method
- `forest-cli attach` calls should be removed, and tracked under #4141
- Progress:
- #4139
- #4154
- #4155
- #4158
- #4168
- #4175
- #4187
- #4191
- #4200
- #4203
- #4204
- #4225
- #4244
- #4248
- #4141 / #4358
- [x] Support aliases: https://github.com/ChainSafe/forest/pull/4155#discussion_r1556026830
- [ ] `crate::rpc_client::RpcRequest::params` should be `Vec` or `RequestParameters`, and should have better error handling.
- [ ] Remove inappropriate impls of `HasLotusJson`
- [ ] Tuples should NOT implement `HasLotusJson`
- [ ] Most types should NOT implement `HasLotusJson`
- https://github.com/ChainSafe/forest/blob/f58b629c79cb26a67bfc48f1ccbc07e2e86e1c38/src/rpc/mpool_api.rs#L136
Note that only the first parameter is wrapped in `LotusJson` - it has separate CBOR and JSON representations.
`MessageSendSpec`, on the other hand, can just be deserialized normally.
A misapplication of the `HasLotusJson` API has lead to the proliferation of `lotus_json_with_self!` calls
- [ ] `LotusJson>` should NOT recurse: users should use `LotusJson>`
- [ ] `LotusJson` is wrong
- #4277
- [ ] Reconsider const generic on `trait RpcMethod`
- [x] Make type names in `OpenRPC` document nice.
- [ ] [Unconsolidate `crate::rpc::types`](https://github.com/ChainSafe/forest/blob/29db7bcfb94ececfd3ae03185aea66e29442ede2/src/rpc/types.rs#L6)
- [x] Autogenerate CLI, maybe JS bindings
- #4295
- [ ] Autogenerate implementation report as discussed in #4129

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.