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

Add a max piece size to storage asks #188

Merged
merged 3 commits into from
Apr 13, 2020
Merged

Add a max piece size to storage asks #188

merged 3 commits into from
Apr 13, 2020

Conversation

arajasek
Copy link
Collaborator

@arajasek arajasek commented Apr 8, 2020

We should probably put something in that ensures that the new max piece size is less than or equal to the sector size.

This PR also includes a commit that makes the Storage Market client refuse to propose a deal if piece size > sector size.

@arajasek arajasek self-assigned this Apr 8, 2020
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.

See comment about functional options pattern -- this will allow us to support more features on the StorageAsk in the future

@@ -262,8 +262,8 @@ func (p *Provider) ListLocalDeals() ([]storagemarket.MinerDeal, error) {
return out, nil
}

func (p *Provider) AddAsk(price abi.TokenAmount, duration abi.ChainEpoch) error {
return p.storedAsk.AddAsk(price, duration)
func (p *Provider) AddAsk(price abi.TokenAmount, duration abi.ChainEpoch, bounds ...abi.PaddedPieceSize) error {
Copy link
Collaborator

@hannahhoward hannahhoward Apr 8, 2020

Choose a reason for hiding this comment

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

So I actually had something slightly different in mind when I mentioned variadic arguments -- I was talking about the functional options pattern:
https://www.sohamkamani.com/golang/options-pattern/

I would define the function this way:

type StorageAskOption func(*storagemarket.StorageAsk)
func (p *Provider) AddAsk(price abi.TokenAmount, duration abi.ChainEpoch, options ...StorageAskOption)

and then you can define:

func MinPieceSize(minPieceSize abi.PaddedPieceSize) StorageAskOptions {
   return func(sa *storagemarket.StorageAsk) {
      sa.MinPieceSize = minPieceSize
   }
}

and so on for other options (leave the price + duration as fixed arguments -- those are always required)

And then, in storedAsk.AddAsk, you can just do:

ask := &storagemarket.StorageAsk{
		Price:        price,
		Timestamp:    height,
		Expiry:       height + duration,
		Miner:        s.actor,
		SeqNo:        seqno,
		MinPieceSize: defaultMinPieceSize,
                  MaxPieceSize: defaultMaxPieceSize,
}
for _, option := range options {
   option(ask)
}

@@ -204,6 +205,10 @@ func (c *Client) ProposeStorageDeal(
return nil, xerrors.Errorf("computing commP failed: %w", err)
}

if uint64(pieceSize.Padded()) > info.SectorSize {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Great idea to do this check early -- even if the provider would reject the deal eventually we should go ahead and error if we know the deal won't be accepted

@arajasek
Copy link
Collaborator Author

Ah, my bad, I definitely didn't know what functional options meant! Thanks for the detailed explanation, I've implemented it (and like the pattern, it's much better than what I had)!

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.

Hey was about to approve this but looks like you have an import cycle in your code. I would move StorageAskOption up to storagemarket/types.go and also change the signature for the function on the interface.

@codecov-io
Copy link

Codecov Report

Merging #188 into master will decrease coverage by 0.32%.
The diff coverage is 23.53%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #188      +/-   ##
==========================================
- Coverage   68.00%   67.68%   -0.31%     
==========================================
  Files          39       39              
  Lines        2065     2079      +14     
==========================================
+ Hits         1404     1407       +3     
- Misses        559      569      +10     
- Partials      102      103       +1     
Impacted Files Coverage Δ
storagemarket/impl/client.go 0.00% <0.00%> (ø)
storagemarket/impl/provider.go 6.99% <0.00%> (ø)
...oragemarket/impl/providerstates/provider_states.go 87.65% <0.00%> (-1.57%) ⬇️
storagemarket/types.go 33.34% <0.00%> (-66.66%) ⬇️
storagemarket/impl/storedask/storedask.go 80.52% <100.00%> (+0.79%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 731e6ca...5b9acf9. Read the comment docs.

@hannahhoward hannahhoward merged commit e232e40 into master Apr 13, 2020
@arajasek arajasek deleted the asr/checksize branch April 24, 2020 18:13
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