Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(api): v2 API integrating with F3 #12719

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

feat(api): v2 API integrating with F3 #12719

wants to merge 5 commits into from

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Nov 25, 2024

WIP but opening this for discussion purposes.

  • To explore how we could evolve the existing APIs I've started a /v2 API set here, although I'm not committing to pursuing that as the solution, it just seems like an appropriate thing to do as long as we're messing with a bunch of standard methods. We could take a minimal approach of adding just the basics to begin with and then adding more as they prove to be necessary. The Common Node API seems like a good foundation to start with.
  • I have two, probably mutually exclusive approaches in here, there's also other ways we could do similar things.
    1. The main one I've gone with is to introduce an EpochSelector string type that's either "latest" or "finalized" and then in places it can either be:
    • An optional argument where none previously existed (e.g. ChainHead() or ChainHead("finalized")).
    • An overload for some methods where TipSetKey currently exists - so you can supply either a tsk or one of these strings in that position.
    • An overload for some methods where ChainEpoch currently exists - so you can supply an integer or one of these strings in that position.
    1. Allowed "latest" and "finalized" to be JSON decoded into a value TipSetKey. "latest" comes out as an EmptyTSK which is the same as providing [] now. "finalized" comes out as a sentinel FinalizedTSK, which is a special TSK comprising ["bafkqaclgnfxgc3djpjswi"] (raw identity CID with the string "finalized"). This approach would allow us to leave the existing APIs unmolested and even work within /v1 transparently. Similar treatment could be done for abi.ChainEpoch but that's a little bit trickier because of the need for a sentinel value, what would that be? Also the optional argument would be a breaking Go API change if we wanted to add something to ChainHead and ChainNotify.

@rvagg rvagg changed the title F3 API exploration feat(api): v2 API integrating with F3 Dec 3, 2024
@rvagg
Copy link
Member Author

rvagg commented Dec 3, 2024

I updated this to lean in to the /v2/ API path and leave /v1/ as it is. The code now publishes a /v2/ and it's so far populated with these:

  • ChainNotify(ctx context.Context, p jsonrpc.RawParams) (<-chan []*api.HeadChange, error) (currently not integrated with F3 for the "finalized" variant)
  • ChainHead(ctx context.Context, p jsonrpc.RawParams) (*types.TipSet, error)
  • ChainGetMessagesInTipset(ctx context.Context, tss types.TipSetSelector) ([]api.Message, error)
  • ChainGetTipSetByHeight(ctx context.Context, epoch abi.ChainEpoch, tss types.TipSetSelector) (*types.TipSet, error)
  • ChainGetTipSetAfterHeight(ctx context.Context, epoch abi.ChainEpoch, tss types.TipSetSelector) (*types.TipSet, error)

The first two take an optional parameter (their /v1 counterparts take no parameters. Calling from Go can supply nil for that second parameter and it'll be the same as v1. The parameter can be one of "latest" or "finalized". "" is like nil, defaulting to "latest".

The others take a types.TipSetSelector which is a wrapper around a union of types.TipSetKey and types.EpochDescriptor and the JSON handling lets it accept the same as the /v1/ APIs or "latest" or "finalized". "" is the same as "latest" which is also the same as [] (types.EmptyTSK). In Go, you either types.NewTipSetSelector(types.EmptyTSK) (or with a concrete TipSetKey value), or use one of types.TipSetSelectorLatest or types.TipSetSelectorFinalized.

But you can see how the pattern would extend. You'd get to something like WalletBalance which only currently takes an address and fetches from the chain head state, we'd add an optional parameter so you can do "finalized".

Basic itests are in place to exercise some of this. So far I haven't set it up to exercise F3 doing its thing (though it should be wired up to work when I get a mock F3 instance going in the itest).


I've listed all of the "Common Node API" methods in a TODO in the v2 docs, but the best approach might be to pick off the most logical F3 APIs and push this out as an "experimental" feature you can turn on and then flesh it out further over time. I'm a little afraid of this PR getting too large to reasonably review.

@masih
Copy link
Member

masih commented Dec 3, 2024

For the record: I continue to believe that the push style ChainNotify API is expensive to implement/maintain with unclear benefits to the users (e.g. they still have to implement reconnect on connection loss, delivery guarantees are AFAIK not spelt out and so on).

My recommendation would be to move to a pull model unless there is a very clear usecase that requires concretely defined "event delivery guarantees". This needs a deeper Product Research. If you are a user of this API/building atop it: please reach out.

@rvagg
Copy link
Member Author

rvagg commented Dec 3, 2024

We can opt to not put some of these methods onto the gateway. Currently /v0/ and /v1/ both expose ChainNotify but we can easily opt to not add it to /v2 and see if anyone squeals; that would be an interesting signal of usage. @masih you might also want to drop a note inline in filecoin-project/FIPs#1027 about your concerns; excluding it from the Common Node API might be reasonable.

@masih
Copy link
Member

masih commented Dec 3, 2024

you might also want to drop a note inline in filecoin-project/FIPs#1027 about your concerns

Done. Thanks for pointing out that FIP.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 📌 Triage
Development

Successfully merging this pull request may close these issues.

2 participants