[Proposal] Simplify ClientRetrieveAPIs to mirror storage

[hannahhoward]

Is your feature request related to a problem? Please describe.

Currently, we have apis for initiating client retrievals:

	// ClientRetrieve initiates the retrieval of a file, as specified in the order.
	ClientRetrieve(ctx context.Context, order api.RetrievalOrder, ref *api.FileRef) error //perm:admin
	// ClientRetrieveWithEvents initiates the retrieval of a file, as specified in the order, and provides a channel
	// of status updates.
	ClientRetrieveWithEvents(ctx context.Context, order api.RetrievalOrder, ref *api.FileRef) (<-chan marketevents.RetrievalEvent, error) //perm:admin

These functions do A LOT:

• They initiate the retrieval
• They also consume events relating to retrieval until the deal is done
• They export the output file to the passed in file ref.
• They transparently clear out any blockstore space used

The APIs also convert the stream of events generated by go-fil-markets to a custom stream, in the process dropping some important data (like the retrieval deal ID). The only difference between the two methods is one version returns the custom event stream while the other consumes said stream to then end and returns the final error.

Finally, these APIs look quite different from storage APIs, which provide:

• function to import data
• function to simply initiate a deal (but not wait for it do finish)
• function to get all deal updates
• function to list deals

Describe the solution you'd like

Per #6337 we now have functions that mirror two of the four storage functions:

• function to get all retrieval deal updates - ClientGetRetrievalUpdates
• function to list all retrievals - ClientListRetrievals

My proposal is to remove the two above APIs from the V1 API and provide two much simpler functions:

	// ClientExport exports a file stored in the local filestore to a system file
	ClientExport(ctx context.Context, exportRef ExportRef, fileRef FileRef) error //perm:admin
	ClientRetrieve(ctx context.Context, params RetrievalParams) (*RestrievalRes, error) //perm:admin

The types referenced are:

// RetrievalParams is RetrievalOrder minus the "LocalStore" param
type RetrievalParams struct {
	// TODO: make this less unixfs specific
	Root                    cid.Cid
	Piece                   *cid.Cid
	Size                    uint64
	Total                   types.BigInt
	UnsealPrice             types.BigInt
	PaymentInterval         uint64
	PaymentIntervalIncrease uint64
	Client                  address.Address
	Miner                   address.Address
	MinerPeer               *retrievalmarket.RetrievalPeer
}

type RestrievalRes struct {
	DealID  retrievalmarket.DealID
	StoreID *multistore.StoreID
}

type ExportRef struct {
	Root    cid.Cid
	StoreID *multistore.StoreID
}

The V0 API could continue to offer the old methods, backed by the functions above, plus ClientGetRetrievalUpdates and ClientRemoveImport (to clear out blockstore data)

Describe alternatives you've considered
Adding a DealID to the event stream returned by ClientRetrieveWithEvents, a simpler solution one way my team is block, but one which really doesn't fix the API asymmetry and how complicated the retrieval methods currently are

Additional context

There is one potential gotcha:
ClientGetRetrievalUpdates returns a struct that is missing one field from the custom stream returned by ClientRetrieveWithEvents who data type is marketevents.Event -- an "Event" field which references the internal go-fil-markets event that triggered the update. This is currently included in the CLI output, though I don't think it's very informative and I'd be surprised if anyone else is using it. We could accept the breaking change of dropping this parameter, or add it to the return channel for ClientGetRetrievalUpdates

This also allows us to remove the retrieval store manager.

We might potentially consider CLI changes as well but as a starting point we'd just reimplement the CLI with the new APIs.

Also, this does cause a number of changes to integration tests and lotus-soup, plus we can optionally rewrite the CLI command. I've actually implemented a lot of this change but don't want to spend more time without general approval of the direction

[dirkmc]

@hannahhoward the proposal seems reasonable to me.

I have a couple of questions:

• Is there something that you can't do with the current APIs that the new APIs would allow you to do?
• I think it may still be worth keeping the existing methods, particularly as this is the most common path and they do the cleanup for you
• Is there a way to use the new APIs to watch for events, without missing any?

[jennijuju]

@arajasek @magik6k @raulk will review this soon!

[willscott]

I ran into this again recently. would be great to get consensus and see if we could simplify&standardize this API

[hannahhoward]

To respond to @dirkmc 's questions:

• the current API lacks a retrieval deal ID -- arguably we could add this without changing the current API strict. I also envision scenarios were we want to watch events for multiple deals. The asymetry of the structure with storage market I also find confusing. The API ends with a phantom extra event that is the ultimate result of the request -- but isn't emmitted by go-fil-markets itself. -- I think this is odd.
• my intent was to keep the existing methods on the v0 api. I don't generally like them as API methods for the future. I get there may be some value, but I consider that value to be to command line consumers, not API consumers who are writing applications that communicate to lotus. I would rather given these folks consistency and predictability, which is what I think aligning with the storage market does
• yes, you setup listening in the same way the code does internally and the same way we do for the storage market in dealbot: you start listening before you propose the deal.

[hannahhoward]

@dircmc @raulk for follow-up.

[hannahhoward]

@dirkmc one more follow-up -- this also allows us to add a --watch option to the lotus client list-retrievals command, which we don't currently have. ClientGetRetrievalUpdates is useful for anyone who wants to monitor deal updates for all deals in real time.

[hannahhoward]

I went ahead and implemented a PR for this proposal, mostly so its off my plate: #6702

[rjan90]

Closing this proposal as these APIs has been removed as part of removing the Legacy Lotus/Lotus-Miner Markets sub-system, which reached EOL at the end of the 31st January 2023, and has been completely removed from the code:

• cleanup: Lotus client: remove markets and deal-making from Lotus Client #11999
• cleanup: miner: remove markets and deal-making from Lotus Miner #12005