-
Notifications
You must be signed in to change notification settings - Fork 59
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
Storage Market Changes Based On Lotus Integration #223
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just had the one question, assuming it's not a bug.
} | ||
|
||
return ctx.Trigger(storagemarket.ProviderEventDealPublishError, xerrors.Errorf("PublishStorageDeals exit code: %w", code)) | ||
if err := environment.Disconnect(deal.ProposalCid); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious about Disconnect. Why is Disconnect called only when is no error? Also, there is a Disconnect call in SendSignedResponse, although in the tests it shows that it's harmless to do it twice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Disconnect gets called in the DealFailing handler, so no need to call it here. It probably should not be calling disconnect in SendSignedResponse and I will check that and add a ticket if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me.
update go-ipfs-ds-help and go-ipfs-blockstore to match lotus
Modify WaitForMessage signature -- as it turns out, we cannot wait for messages with confidences, only state changes (for the most part)
add a context to WaitForMessage since generally wait apis on node expect a context
55a1998
to
1738687
Compare
Goals
Get the latest go-fil-markets integrated with lotus so we can cut a 0.2.0 branch
Implementation
The integration revealed we can't as it turns out, wait with confidence on a chain message, only a state change -- we could technically call StateGetReceipt or StateSearchMessage -- but this would likely be extremely slow :(
So, in the absence, I removed confidence for now.
Also, our callback takes an exit code and data, but does not account for the possibility that the wait call itself errors (i.e. message was never posted). So I added third error parameter to the callback to capture this.
I also added a context to the call (LOTUS and I imagine GFC API expect a context), and made a few dependency updates