Import spec actor types #118

hannahhoward · 2020-02-07T00:39:04Z

Goals

Use spec actor types, unifying various go actor types

Implementation

Replace tokenamount.Tokenamount with TokenAmount from specs-actors abi package. Use specs-actors big type for operations
Remove types.Signature, replace with Signature from specs-actors crypto package. Since this signature intentionally lacks a Verify function, add it as a function provided by the node implementation interface, which makes sense anyway since the node should have its own implementation of signature as it's a security critical feature
Remove types.SignedVoucher, replace with SignedVoucher from specs-actors paych package
Move types.SignedStorageAsk to storagemarket package, since this is the only place it's used and moreover, it's something that's unique to the shared markets as asks are not on chain
Remove shared folder entirely
Replace storagemarket.StorageDealProposal with spec-actors ClientDealProposal + Deal Proposal. This also necessitates switching to real PieceCIDs, which we handle with the go-fil-commcid package

codecov-io · 2020-02-11T00:56:06Z

Codecov Report

Merging #118 into master will increase coverage by 1.91%.
The diff coverage is 14.81%.

@@            Coverage Diff             @@
##           master     #118      +/-   ##
==========================================
+ Coverage   35.86%   37.77%   +1.91%     
==========================================
  Files          43       35       -8     
  Lines        3188     2619     -569     
==========================================
- Hits         1143      989     -154     
+ Misses       1730     1367     -363     
+ Partials      315      263      -52

Impacted Files	Coverage Δ
storagemarket/impl/provider.go	`0% <0%> (ø)`	⬆️
storagemarket/impl/provider_states.go	`0% <0%> (ø)`	⬆️
storagemarket/impl/client_cbor_gen.go	`5.18% <0%> (-1.8%)`	⬇️
storagemarket/impl/client_storagemarket.go	`0% <0%> (ø)`	⬆️
storagemarket/impl/client_states.go	`0% <0%> (ø)`	⬆️
storagemarket/impl/provider_storagemarket.go	`0% <0%> (ø)`	⬆️
storagemarket/impl/client.go	`0% <0%> (ø)`	⬆️
storagemarket/impl/provider_asks.go	`0% <0%> (ø)`	⬆️
...ievalmarket/impl/providerstates/provider_states.go	`94.32% <100%> (ø)`	⬆️
retrievalmarket/impl/clientstates/client_states.go	`99.02% <100%> (ø)`	⬆️
... and 10 more

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 e814d5d...0e52ddf. Read the comment docs.

ingar

Awesome. I mainly looked at storagemarket stuff, just a few comments.

Another general comment: seems like we are inconsistent with our usage of Miner and Provider. We should probably choose one and stick with it.

ingar · 2020-02-12T19:24:51Z

storagemarket/types.go

@@ -217,11 +166,14 @@ type StorageProvider interface {
 type StorageProviderNode interface {
 	MostRecentStateId(ctx context.Context) (StateKey, error)

+	// Verify a signature against an address + data
+	VerifySignature(signature crypto.Signature, signer address.Address, plaintext []byte) bool


ingar · 2020-02-12T19:25:14Z

storagemarket/integration_test.go

@@ -128,6 +132,121 @@ func TestMakeDeal(t *testing.T) {
 	assert.Equal(t, pd.State, storagemarket.StorageDealActive)
 }

+func TestMakeDealOffline(t *testing.T) {


ingar · 2020-02-12T19:39:50Z

storagemarket/impl/client.go

-		PieceRef:             commP,
-		PieceSize:            uint64(pieceSize),
+	dealProposal := market.DealProposal{
+		PieceCID:             commcid.PieceCommitmentV1ToCID(commP),


Does it make sense to have PieceIO.GeneratePieceCommitment return a cid.CID? Would remove a little boilerplate from client and provider.

hmmm... good question. Let's leave it for a future discussion

ingar · 2020-02-12T19:42:57Z

storagemarket/impl/provider_states.go

@@ -45,13 +46,13 @@ func (p *Provider) validating(ctx context.Context, deal MinerDeal) (func(*MinerD
 	if err != nil {
 		return nil, err
 	}
-	if head.Height() >= deal.Proposal.ProposalExpiration {
+	if head.Height() >= deal.Proposal.StartEpoch {


Not really related to this PR, but this check doesn't give us any slack time to seal/commit a sector before the StartEpoch.

yea we should make decision about that in the future

shannonwells

Just a couple of questions, no blockers

shannonwells · 2020-02-12T18:30:54Z

retrievalmarket/impl/clientstates/client_states_test.go


 func makeDealState(status retrievalmarket.DealStatus) *retrievalmarket.ClientDealState {
 	return &retrievalmarket.ClientDealState{
 		TotalFunds:       defaultTotalFunds,
 		MinerWallet:      address.TestAddress,
 		ClientWallet:     address.TestAddress2,
 		PayCh:            address.TestAddress2,
-		Lane:             uint64(10),
+		Lane:             int64(10),


Note that now we need to make sure that Lanes are checked for <0 as well as any other place where a negative value would be bad.

I'd expect this to be changed back to uint64 in the spec

This being int64 is a spec bug

yea I dunno what to do about this. I wish we weren't using signed ints in spec in so many places.

filecoin-project/specs-actors@e2abd13

shannonwells · 2020-02-12T18:55:25Z

retrievalmarket/types.go

 	ListDeals() map[ProviderDealID]ProviderDealState
 }

 // RetrievalProviderNode are the node depedencies for a RetrevalProvider
 type RetrievalProviderNode interface {
-	UnsealSector(ctx context.Context, sectorId uint64, offset uint64, length uint64) (io.ReadCloser, error)
-	SavePaymentVoucher(ctx context.Context, paymentChannel address.Address, voucher *types.SignedVoucher, proof []byte, expectedAmount tokenamount.TokenAmount) (tokenamount.TokenAmount, error)
+	UnsealSector(ctx context.Context, sectorID uint64, offset uint64, length uint64) (io.ReadCloser, error)


linters, amirite?

shannonwells · 2020-02-12T19:22:59Z

storagemarket/impl/client_storagemarket.go

-		MinerID:            info.PeerID,
+		Data:            data,
+		PricePerEpoch:   price,
+		StartEpoch:      startEpoch,


We'll need to make sure we guard against negative epochs now too.

I feel like the check to see if StartEpoch > current chain height protects against that, but I wish we were using unsigned ints in spec

shannonwells · 2020-02-12T19:35:30Z

storagemarket/impl/provider.go

 	_, err = tempfi.Seek(0, io.SeekStart)
 	if err != nil {
 		return xerrors.Errorf("failed to seek through temp imported file: %w", err)
 	}

-	commP, err := p.pio.ReadPiece(tempfi)


is p.pio used anymore?

it is. elsewhere

Switch to the tokenamount in the spec-actor repo and remove tokenamount.TokenAmount

move ask to storage market, import voucher & signature from specs-actors

Just remove 2 references to a single constant that does not have the same meaning any more

use chains deal proposal and storage deal proposal, switch to using PieceCIDs, switch storageAsk to chain epoch based, and have the piece store be keyed by CIDs as well.

use embedded struct to simplify property access

add an integration test to verify the offline deal flow is working

hannahhoward force-pushed the feat/spec-actor-types branch from 33d4b5b to c327a27 Compare February 10, 2020 23:53

hannahhoward changed the title ~~WIP: Import spec actor types~~ Import spec actor types Feb 10, 2020

hannahhoward requested review from ingar and shannonwells February 11, 2020 01:19

hannahhoward force-pushed the feat/spec-actor-types branch from 0e52ddf to 7c7c4f8 Compare February 11, 2020 03:28

ingar approved these changes Feb 12, 2020

View reviewed changes

shannonwells approved these changes Feb 12, 2020

View reviewed changes

hannahhoward added 6 commits February 12, 2020 11:59

feat(types): switch to spec actor big.Int

744d262

Switch to the tokenamount in the spec-actor repo and remove tokenamount.TokenAmount

refactor(types): remove shared types

4b7fb56

move ask to storage market, import voucher & signature from specs-actors

refactor(types): remove shared folder entirely

349c3d1

Just remove 2 references to a single constant that does not have the same meaning any more

feat(storagemarket): import chain deal types

f4146dc

use chains deal proposal and storage deal proposal, switch to using PieceCIDs, switch storageAsk to chain epoch based, and have the piece store be keyed by CIDs as well.

refactor(storagemarket): simplify deal access

52906d5

use embedded struct to simplify property access

test(storagemarket): integration test offline deals

583d217

add an integration test to verify the offline deal flow is working

hannahhoward force-pushed the feat/spec-actor-types branch from 7c7c4f8 to bf39448 Compare February 12, 2020 19:59

fix(lint): fix lint errors

ddf6b73

hannahhoward force-pushed the feat/spec-actor-types branch from bf39448 to ddf6b73 Compare February 12, 2020 20:03

hannahhoward merged commit a7bad90 into master Feb 12, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Import spec actor types #118

Import spec actor types #118

hannahhoward commented Feb 7, 2020 •

edited

codecov-io commented Feb 11, 2020

ingar left a comment

ingar Feb 12, 2020

ingar Feb 12, 2020

ingar Feb 12, 2020

hannahhoward Feb 12, 2020

ingar Feb 12, 2020

hannahhoward Feb 12, 2020

shannonwells left a comment

shannonwells Feb 12, 2020

magik6k Feb 12, 2020

magik6k Feb 12, 2020

hannahhoward Feb 12, 2020

magik6k Feb 12, 2020

shannonwells Feb 12, 2020

hannahhoward Feb 12, 2020

shannonwells Feb 12, 2020

hannahhoward Feb 12, 2020

shannonwells Feb 12, 2020

hannahhoward Feb 12, 2020

Import spec actor types #118

Import spec actor types #118

Conversation

hannahhoward commented Feb 7, 2020 • edited

Goals

Implementation

codecov-io commented Feb 11, 2020

Codecov Report

ingar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shannonwells left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hannahhoward commented Feb 7, 2020 •

edited