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

Improve offline deals #183

Merged
merged 1 commit into from
Apr 6, 2020
Merged

Improve offline deals #183

merged 1 commit into from
Apr 6, 2020

Conversation

arajasek
Copy link
Collaborator

@arajasek arajasek commented Apr 6, 2020

With these changes, the offline deal workflow succeeds

@@ -426,6 +426,9 @@ type StorageClient interface {
// ProposeStorageDeal initiates deal negotiation with a Storage Provider
ProposeStorageDeal(ctx context.Context, addr address.Address, info *StorageProviderInfo, data *DataRef, startEpoch abi.ChainEpoch, endEpoch abi.ChainEpoch, price abi.TokenAmount, collateral abi.TokenAmount, rt abi.RegisteredProof) (*ProposeStorageDealResult, error)

// CalculateCommP calculates the piece CID for a given CAR file and miner
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment seems slightly inaccurate

Copy link
Collaborator

@hannahhoward hannahhoward left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arajasek generally LGTM

One quick question: pieceio.GeneratePieceCommitment is a static, public function, that can be accessed outside go-fil-markets, so I'm not sure about adding this to the storage client as a function: as in the body of the function client.CalculateCommP actually contains (I believe) no references to the actual data variables of the client struct, so my inclination would be to move the contents of this code into the node implementation, to keep the API surface of the client limited only to being for making deals.

Total aside: having lotus make the CAR files and generate commP itself -- does this actually work with the workflow for the team generating the data?

var pieceSize abi.UnpaddedPieceSize
var err error

if data.TransferType != storagemarket.TTManual {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the clientutils.CommP function already checks if its a manual transfer and short circuits. Maybe move this logic down into that?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea that makes sense

@arajasek
Copy link
Collaborator Author

arajasek commented Apr 6, 2020

Thanks for the suggestions, @hannahhoward, I didn't realise after some edits that I wound up just calling a public static method! I've switched to doing it directly from Lotus, I think it's nicer :)

All suggested changes implemented :)

@whyrusleeping whyrusleeping merged commit 412b34d into master Apr 6, 2020
@whyrusleeping whyrusleeping deleted the asr/api branch April 6, 2020 21:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants