Integrate storage market provider from go-fil-markets #3737
Conversation
internal/app/go-filecoin/internal/submodule/storage_protocol_submodule.go
Outdated
Show resolved
Hide resolved
internal/app/go-filecoin/internal/submodule/storage_protocol_submodule.go
Outdated
Show resolved
Hide resolved
internal/app/go-filecoin/internal/submodule/storage_protocol_submodule.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Good to merge.
My two most difficult opinions are:
-
it is unfortunate that this work seems to be branching off in the same direction as porcelain but with different code and it would be good to imagine what we should do about this PR setting a precedent for duplicating that part of the system. If we continue to duplicate porcelain we should try to generalize the duplicated area and deprecate and delete porcelain code because maintaining two copies is bad. If want to double down on porcelain we could use it to implement the common connector and shove new functionality down there.
-
any code to handle differences between the address types of markets and gfc is a step in the wrong direction not worth the cost-- it should be very easy to switch gfc to use the shared addresses (because it is just gfc code copy pasted) in a single large and tedious but mechanical commit.
| @@ -44,8 +44,11 @@ type ChainMessage struct { | |||
| Receipt *types.MessageReceipt | |||
| } | |||
|
|
|||
| return &stateKey{key, uint64(ts.At(0).Height)}, nil | ||
| } | ||
|
|
||
| func (c *connectorCommon) wait(ctx context.Context, mcid cid.Cid, pubErrCh chan error) (*types.MessageReceipt, error) { |
There was a problem hiding this comment.
Because this PR has opted to circumvent the porcelain abstraction it ends up duplicating existing work like this in porcelain (while you're impl is nicer because it handles timeouts, it would have been better to improve existing and consolidate.)
This is a bummer all around so I'll be keeping my eyes out for some way to consolidate duplication among porcelain and connectors. Did you consider giving the connectorCommon a (subset of a?)porcelainAPI handle?
|
|
||
| proposal := msgProposals[0] // TODO: Support more than one deal | ||
|
|
||
| // TODO: find a better way to do this |
There was a problem hiding this comment.
Would reflect.DeepEqual help here? If not seems like writing a custom Equals or CompareForValidation method on proposal would be nice. Seems like it shouldn't have to wait for after this PR.
|
Drive-by opinion: I favour us moving away from porcelain, so if there's a path to deleting the duplicated concepts, 👍 for landing new ones. |
Codecov Report
@@ Coverage Diff @@
## master #3737 +/- ##
=======================================
- Coverage 39% 39% -1%
=======================================
Files 255 246 -9
Lines 14602 14353 -249
=======================================
- Hits 5750 5609 -141
+ Misses 8144 8051 -93
+ Partials 708 693 -15 |
acruikshank
force-pushed
the
feat/storage-provider-node-impl
branch
from
9d11cb7 to
be0287f
Compare
acruikshank
force-pushed
the
feat/storage-provider-node-impl
branch
from
be0287f to
369a97f
Compare
|
Thank you! |
Motivation
The
go-fil-marketsproject implements a spec compliant version of the storage market. We would like to use this code in go-filecoin to bring our storage market operations up to spec.Proposed changes
This PR brings in go-fil-markets and wires it into go-filecoin by implementing the storage provider API and storage client API. Wiring up the retrieval market or the retrieval client is out of scope for this PR, and some retrieval code has been commented out to make this PR work. Since we lack the actor support to make the storage market work, a fully operational storage market is also out of scope. Consequently, we skipped and commented out some tests to allow us to continue building prior to making the storage market functional. We would eventually like these tests to. work with the new code.
As a consequence of integrating go-fil-markets, we upgrade ipld-prime and graphsync. This has various ramifications throughout the code.
closes #3719