I can live with this I think. I don't love the verbosity of the nesting but at least you've come up with short descriptors, "tag" and "at", I like that 👍. Good stuff.

Unrelated flaky itest recorded @ #13005

Initial quick skim:

• Height is a bit of a miss-nomer, we don't track height but an epoch in filecoin as height originally refers to the height of the hash tree.
• Why the generics complexity logic in the type? Why not use the TipSetCriterion directly with additional validation checks in the API?

Discussed at stand up:

Height is a bit of a miss-nomer, we don't track height but an epoch in filecoin as height originally refers to the height of the hash tree.

It is. From internal notion doc discussions I got the sense that we want to switch to "height" as the new terminology. Happy to revert back to Epoch, or move on with Epoch. Either works.

@Kubuxu kindly pointed out that the Go SDK to interact with selectors is slightly cumbersome. I am open to suggestions on this one and I think we can iterate on it as non-breaking API changes later. For now I am keen to pin down the primitives for the Lotus V2 API first and then iterate.

Why the generics complexity logic in the type? Why not use the TipSetCriterion directly with additional validation checks in the API?

The generics is used solely as Type Constraint to NewTipSetSelector. We can't use TipSetCriterion as argument to ChainGetTipSet directly, because it is not a type alias of jsonrpc.RawParams without losing the ability to accept requests with no params specified. This is a limitation inherited by the implementation of jsonrpc library used by Lotus.

During the discussion, the choice of default param as "Heaviest" if nothing is specified came up:

• @Kubuxu mentioned that maybe we should explicitly ask for what the user wants and have no default if no selector is specified.
• I have no strong opinions on the defaults. I do see that explicit is better ( which is the rationale behind changes here compared to prior iterations). However, we would lose the "short-and-sweet" aspect of API calls by users in other languages, a non-functional requirement for the v2 API as I understand it.

@Kubuxu please add anything that I might have missed.

@rvagg thoughts?

Discussed it a bit with Masih.

1. The weird typing is required if we want to have Filecoin.ChainGetTipSet with no params to work across JSOR-RPC
2. IDK if I like that you will be required to type,

ts, err := api.ChainGetTipSet(ctx, types.NewTipSetSelector(types.TipSetHeight{
	At:       123,
	Previous: true,
	Anchor: &types.TipSetAnchor{
		Tag: &types.TipSetTags.Finalized,
		Key: &types.TipSetKey{},
	},
}))

3. I'm 90% that the latest on empty is not what we want. Split across "empty invalid" and empty returning finalized.

written before Masih wore the above ^^

I'm 90% that the latest on empty is not what we want. Split across "empty invalid" and empty returning finalized.

(Not an expert opinion here), but I agree. I think it's important to help builders "avoid running into trouble unknowingly". We can do this by either (as Kuba says):

1. Be explicit - no default
2. Use finalized

That said, maybe best to match ETH ecosystem here? Looking at getBlockByNumber (which I assume is a good corollary), it looks like there is no default.

Going ahead with no default.

IDK if I like that you will be required to type

Let's see what things look like after explicit param is needed and we can iterate. Probably faster to sit down and go through it synchronously to distill actionables on this one.

@Kubuxu Do you want to get rid of the default anchor for getting tipset by height too or leave it as optional?

@Kubuxu

• A selector is now strictly required; no defaults.
• We can't get rid of the type alias to jsonrpc.RawParams without making request body more ugly:
  • The jsonrpc library lotus uses would implicitly switch to using JSON array as parms if the type of an argument is not jsonrpc.RawParams.
  • This means request params on the wire would have to have an array wrapping, e.g.: {"jsonrpc":"2.0","method":"Filecoin.ChainGetTipSet","params":[{"tag":"finalized"}],"id":1} instead of {"jsonrpc":"2.0","method":"Filecoin.ChainGetTipSet","params":{"tag":"finalized"},"id":1}.
  • The choices are to: use raw params type alias, live with the redundant wrapping, or fix go-jsonrpc to do things differently.
    • I don't think it makes sense to use the mix-and-match style params experimented with in feat(api): v2 API integrating with F3 #12719 because for me that conflicts with "Explicit APIs" we desire. My vote is to keep the parameters keyed if the API arg needs to be extensible.
  • My pick is to use the the params type alias as it is the most straightforward way to get what we want while keeping the API extensible if we want to add anything else.
• I have not touched the Go API for constructing selectors. This one is mostly bike shedding and best done synchronously.

the params being array is fully expected. It is a single positional argument, which is an object.
I haven't seen the named parameters from JSON-RPC spec used anywhere.
They are allowed by not that popular.

Looks really good, I'll have to try and restart my Eth API work ontop of this for /v2 to remind myself about all the module hackery needed to make this work. But I like how the TipSetSelector has worked out here and my initial take is that the the ChainGetTipSet v2 wrapper is nice and clean, although I wonder how we're going to keep it simple when we need to deal with selecting a tipset for all the other methods (thinking especially the State* ones) that currently take an explicit tipset or EmptyTSK. Perhaps it's the v1 APIs that have to get wrapped to suit the internal v2 methods where a TipSetSelector is what we accept on the internal functions.

how we're going to keep it simple when we need to deal with selecting a tipset for all the other methods (thinking especially the State* ones

Thanks for pointing that out. I will pick up a State API next to flesh out any complexities before making Chain* more featureful.

Perhaps it's the v1 APIs that have to get wrapped to suit the internal v2 methods

This has been my thinking so far: to eventually turn v1 APIs into a wrapper of v2.

@Kubuxu @rvagg can I get a review on this PR so that we can distill what's missing from this PR to land please?

Let's discuss further in stand up (about named parameters).

Inevitably this is going to come up again. I think we should capture the rationale for the decision. I extracted some of the statements I saw to https://www.notion.so/filecoindev/Filecoin-V2-APIs-1d0dc41950c1808b914de5966d501658?pvs=4#1d2dc41950c180019135f921e171e984 but it needs work with happened verbally.