forest icon indicating copy to clipboard operation
forest copied to clipboard

Support different `/rpc/vX` paths in `RpcMethod`

Open elmattic opened this issue 1 year ago • 10 comments

Previous code here and here.

trait RpcMethod assumes that each method will be exposed on one (and only one) endpoint: https://github.com/ChainSafe/forest/blob/07576ded7933fa85ca623eb8ef84c3a4fb77eb22/src/rpc/reflect/mod.rs#L74-L75.

This was fine for a while because methods weren't different between endpoints, and we ignore HTTP paths as a matter of course.

For #4424 and #4038 we need to start being path-aware again:

  • (relatively) low-cost to support v0 methods.
  • prepare our architecture for future API path changes
  • this is required for us to emit OpenRPC documents per-path, which we want to support #4038

This issue covers the design and implementation of this work

  • [x] trait RpcMethod allows multiple paths
    - const API_VERSION: ApiVersion;
    + const API_PATHS: &[ApiPath];
    
    We can const assert for uniqueness and inhabitedness etc where we need (or use nunny::slice![..]).
  • [ ] our server code hoists the correct methods to the correct paths.
  • [ ] forest-tool api compare tests all API_PATHS for each method.
  • [x] our openrpc code segregates by path, and we get rid of our tagging logic
    • [x] forest-tool shed openrpc accepts a path to dump out.

elmattic avatar Feb 16 '24 10:02 elmattic

@lemmih Is this still relevant?

ansermino avatar Apr 16 '24 18:04 ansermino

It improves compatibility with Lotus, and it's simple to implement. We should still pursue this even though it's a low-priority issue.

lemmih avatar Apr 17 '24 07:04 lemmih

This is blocking https://github.com/ChainSafe/forest/issues/4424

Some RPC methods(StateSectorPreCommitInfo, StateWaitMsg) behave differently between v0 and v1 endpoints.

hanabi1224 avatar Jun 21 '24 09:06 hanabi1224

Does it make sense to continue maintaining v0? It may just be technical debt moving forward

ansermino avatar Jun 21 '24 09:06 ansermino

It is still unclear whether the v0 endpoints are required. They are not used by Curio but they may be used by exchanges.

lemmih avatar Aug 02 '24 13:08 lemmih

We will not be pursuing the V0 methods any further. In any case this may still be required in future for V3 and beyond.

This can probably be deprioritized in the meantime (p4?)

ansermino avatar Aug 08 '24 17:08 ansermino

There was related work in this PR:

  • #4516

But our tests failed, due to:

  • schema mismatches
  • Some RPC methods behave differently between v0 and v1 endpoints.

It's important that I get a complete schema to autogenerate from for the test suite.

We will not be pursuing the V0 methods any further

Should we leave the existing methods in @ansermino?

aatifsyed avatar Aug 13 '24 14:08 aatifsyed

Approach for this is to move all possible methods to v1, fixing breaks as we find them

aatifsyed avatar Aug 21 '24 15:08 aatifsyed

@aatifsyed Please also open a tracking issue for V1 incompatible methods

ansermino avatar Aug 21 '24 15:08 ansermino