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.

Sign up for GitHub

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

Jump to bottom

Handle deal id changes in OnDealSectorCommitted #4730

Merged
merged 7 commits into from
Nov 10, 2020
Merged

Handle deal id changes in OnDealSectorCommitted #4730

merged 7 commits into from
Nov 10, 2020

Conversation

hannahhoward
Copy link
Contributor

@hannahhoward hannahhoward commented Nov 5, 2020 •
edited
Loading

Goals

Resolve deal id not found

Implementation

  • Add GetCurrentDealInfo that:
    • attempts to read deal from state with deal ID
    • if unsuccessful, attempts to lookup publish message and get new deal id from return value
    • attempts again to read deal from state with new deal ID
  • Extract OnDealSectorCommitted code so that it's shared
  • Modify OnDealSectorCommitted to handle changes to deal ID in the context PreCommitSector and ProveCommitSector
  • Add a test for OnDealSectorCommitted

Pairs with filecoin-project/go-fil-markets#441

@arajasek
Copy link
Contributor

arajasek commented Nov 6, 2020

Fixes #3566

@arajasek arajasek added this to the 🖇Lotus Stabilization milestone Nov 6, 2020
@jennijuju
Copy link
Member

fixes #3461

@jennijuju jennijuju linked an issue Nov 6, 2020 that may be closed by this pull request
Failed to look up deal on chain #3461
Closed
@magik6k
Copy link
Contributor

magik6k commented Nov 6, 2020

(lint fails)

@hannahhoward
Copy link
Contributor Author

@magik6k re: lint -- yes I know, it seems not to like my table test structure and I'm not sure what to do about it.

The closure in question is the sub test function.

hannahhoward
hannahhoward commented Nov 6, 2020
View reviewed changes
Copy link
Contributor Author

@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.

@magik6k I have some comments about GetCurrentDealInfo and testing the old deal first.

markets/storageadapter/getcurrentdealinfo.go Show resolved Hide resolved
markets/storageadapter/getcurrentdealinfo.go Show resolved Hide resolved
markets/storageadapter/getcurrentdealinfo.go Show resolved Hide resolved
@magik6k magik6k mentioned this pull request Nov 6, 2020
Closed
@arajasek
Copy link
Contributor

arajasek commented Nov 8, 2020

Fixes #3474

whyrusleeping
whyrusleeping reviewed Nov 10, 2020
View reviewed changes
markets/storageadapter/getcurrentdealinfo.go Outdated
func GetCurrentDealInfo(ctx context.Context, ts *types.TipSet, api getCurrentDealInfoAPI, dealID abi.DealID, proposal market.DealProposal, publishCid *cid.Cid) (abi.DealID, *api.MarketDeal, error) {
marketDeal, dealErr := api.StateMarketStorageDeal(ctx, dealID, ts.Key())
if dealErr == nil {
if marketDeal.Proposal == proposal {
Copy link
Member

Choose a reason for hiding this comment

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

Are we sure comparing these like this is correct? no pointers involved or anything fishy like that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope those are both structs. neither have pointers to my knowledge (they're on chain structs, so that would make sense)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

then again, the fact that we're failing the deal flow tests suggests otherwise. will investigate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nice, the Client address gets translated to an actor address in chain state, so this will not work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed

@hannahhoward
feat(markets): handle deal ID changes
d2acc78 
make OnDealSectorCommitted handle changes to deal ids
@hannahhoward
refactor(markets): refactor OnDealSectorCommitted
a3e9355 
extract OnDealSectorCommitted from adapters and test
@hannahhoward
style(lint): fix lint issues
09a0d6e
@hannahhoward
feat(markets): check deal equality
7519bdd 
in OnDealSectorCommitted, verify that deals looked up match the deal proposals which were made
@hannahhoward
style(lint): fix lint issues
b010ac2
@hannahhoward
fix(markets): correct deal equality check
314dda0 
correct comparison of deal equality (a strict == is not enough)
@hannahhoward hannahhoward force-pushed the feat/deal-id-change-in-sector-committed branch from 26f0255 to 314dda0 Compare November 10, 2020 03:24
@hannahhoward
feat(markets): update to 1.0.1
ea726c9 
update to tagged 1.0.1 release & also fix lint error
@hannahhoward hannahhoward mentioned this pull request Nov 10, 2020
Closed
@arajasek
Copy link
Contributor

Fixes #4777 and #4737

@hannahhoward hannahhoward mentioned this pull request Nov 10, 2020
Closed
@magik6k magik6k merged commit ab7ee2f into master Nov 10, 2020
@magik6k magik6k deleted the feat/deal-id-change-in-sector-committed branch November 10, 2020 21:18
This was referenced Nov 10, 2020
Closed
Closed
This was referenced Nov 10, 2020
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@whyrusleeping whyrusleeping whyrusleeping left review comments

@magik6k magik6k magik6k approved these changes

@Kubuxu Kubuxu Awaiting requested review from Kubuxu

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
🖇Lotus Stabilization (1.1.3)
Development

Successfully merging this pull request may close these issues.

Failed to look up deal on chain
5 participants
@hannahhoward @arajasek @jennijuju @magik6k @whyrusleeping