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

Get retrieval pricing input should not error out on a deal state fetch #6679

Merged
merged 3 commits into from
Jul 5, 2021
Merged

Get retrieval pricing input should not error out on a deal state fetch #6679

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8ecdc92
get retrieval pricing input should not error out on a deal state fetch
aarshkshah1992 Jul 5, 2021
4498572
changes as per review
aarshkshah1992 Jul 5, 2021
add113f
use multierror
aarshkshah1992 Jul 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions markets/retrievaladapter/provider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"io"

"github.com/filecoin-project/lotus/api/v1api"
"github.com/hashicorp/go-multierror"
"golang.org/x/xerrors"

"github.com/ipfs/go-cid"
Expand Down Expand Up @@ -135,10 +136,14 @@ func (rpn *retrievalProviderNode) GetRetrievalPricingInput(ctx context.Context,
}
tsk := head.Key()

var mErr error

for _, dealID := range storageDeals {
ds, err := rpn.full.StateMarketStorageDeal(ctx, dealID, tsk)
if err != nil {
return resp, xerrors.Errorf("failed to look up deal %d on chain: err=%w", dealID, err)
log.Warnf("failed to look up deal %d on chain: err=%w", dealID, err)
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest that if attempting to fetch the state for all deals fails, we return the error (instead of just logging it) so it's clearer what the root cause is

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed this. Thanks.

mErr = multierror.Append(mErr, err)
continue
}
if ds.Proposal.VerifiedDeal {
resp.VerifiedDeal = true
Expand All @@ -158,7 +163,11 @@ func (rpn *retrievalProviderNode) GetRetrievalPricingInput(ctx context.Context,
// Note: The piece size can never actually be zero. We only use it to here
// to assert that we didn't find a matching piece.
if resp.PieceSize == 0 {
return resp, xerrors.New("failed to find matching piece")
if mErr == nil {
return resp, xerrors.New("failed to find matching piece")
}

return resp, xerrors.Errorf("failed to fetch storage deal state: %w", mErr)
}

return resp, nil
Expand Down
51 changes: 51 additions & 0 deletions markets/retrievaladapter/provider_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,31 @@ func TestGetPricingInput(t *testing.T) {
expectedErrorStr: "failed to find matching piece",
},

"error when fails to fetch deal state": {
fFnc: func(n *mocks.MockFullNode) {
out1 := &api.MarketDeal{
Proposal: market.DealProposal{
PieceCID: pcid,
PieceSize: paddedSize,
},
}
out2 := &api.MarketDeal{
Proposal: market.DealProposal{
PieceCID: testnet.GenerateCids(1)[0],
VerifiedDeal: true,
},
}

n.EXPECT().ChainHead(gomock.Any()).Return(tsk, nil).Times(1)
gomock.InOrder(
n.EXPECT().StateMarketStorageDeal(gomock.Any(), deals[0], key).Return(out1, xerrors.New("error 1")),
n.EXPECT().StateMarketStorageDeal(gomock.Any(), deals[1], key).Return(out2, xerrors.New("error 2")),
)

},
expectedErrorStr: "failed to fetch storage deal state",
},

"verified is true even if one deal is verified and we get the correct piecesize": {
fFnc: func(n *mocks.MockFullNode) {
out1 := &api.MarketDeal{
Expand All @@ -92,6 +117,32 @@ func TestGetPricingInput(t *testing.T) {
expectedVerified: true,
},

"success even if one deal state fetch errors out but the other deal is verified and has the required piececid": {
fFnc: func(n *mocks.MockFullNode) {
out1 := &api.MarketDeal{
Proposal: market.DealProposal{
PieceCID: testnet.GenerateCids(1)[0],
},
}
out2 := &api.MarketDeal{
Proposal: market.DealProposal{
PieceCID: pcid,
PieceSize: paddedSize,
VerifiedDeal: true,
},
}

n.EXPECT().ChainHead(gomock.Any()).Return(tsk, nil).Times(1)
gomock.InOrder(
n.EXPECT().StateMarketStorageDeal(gomock.Any(), deals[0], key).Return(out1, xerrors.New("some error")),
n.EXPECT().StateMarketStorageDeal(gomock.Any(), deals[1], key).Return(out2, nil),
)

},
expectedPieceSize: unpaddedSize,
expectedVerified: true,
},

"verified is false if both deals are unverified and we get the correct piece size": {
fFnc: func(n *mocks.MockFullNode) {
out1 := &api.MarketDeal{
Expand Down