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

UpdateStorage accepts negative values #2990

Merged
merged 1 commit into from Jun 25, 2019

Conversation

@ZenGround0
Copy link
Contributor

commented Jun 24, 2019

This PR includes tests demonstrating that UpdateStorage already accepts negative values.

It also refactors state machine test helpers so that we can effectively test UpdateStorage which previously was not tested.

Feedback welcome if leveraging possibility of negative values in BytesAmount is too hacky and negative values should be more explicitly created in that type.

"github.com/filecoin-project/go-filecoin/state"
th "github.com/filecoin-project/go-filecoin/testhelpers"
tf "github.com/filecoin-project/go-filecoin/testhelpers/testflags"
"github.com/filecoin-project/go-filecoin/types"
"github.com/filecoin-project/go-filecoin/vm"
)

func createTestMiner(t *testing.T, st state.Tree, vms vm.StorageMap, minerOwnerAddr address.Address, pid peer.ID) address.Address {

This comment has been minimized.

Copy link
@ZenGround0

ZenGround0 Jun 24, 2019

Author Contributor

Moved these to testhelpers so we can use them from within storagemarket_test

@ZenGround0 ZenGround0 force-pushed the feat/2808 branch from 45a2b79 to b4477c0 Jun 24, 2019

)

// MustGetNonce returns the next nonce for an actor at an address or panics.

This comment has been minimized.

Copy link
@ZenGround0

ZenGround0 Jun 24, 2019

Author Contributor

These changes pay down some technical debt. For more discussion see #2992

@@ -185,15 +185,21 @@ func newMessageApplier(smsg *types.SignedMessage, processor *consensus.DefaultPr
return nil, err
}

// CreateAndApplyTestMessage wraps the given parameters in a message and calls ApplyTestMessage
func CreateAndApplyTestMessage(t *testing.T, st state.Tree, vms vm.StorageMap, to address.Address, val, bh uint64, method string, ancestors []types.TipSet, params ...interface{}) (*consensus.ApplicationResult, error) {
// CreateAndApplyTestMessageFrom wraps the given parameters in a message and calls ApplyTestMessage.

This comment has been minimized.

Copy link
@ZenGround0

ZenGround0 Jun 24, 2019

Author Contributor

With new storagemarket tests for the first time the From address of a message has become semantically important so this refactor allows tests to set this field.

@ZenGround0 ZenGround0 requested review from acruikshank and laser Jun 24, 2019

@codecov-io

This comment has been minimized.

Copy link

commented Jun 24, 2019

Codecov Report

Merging #2990 into master will decrease coverage by <1%.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #2990   +/-   ##
======================================
- Coverage      45%     44%   -1%     
======================================
  Files         213     213           
  Lines       12756   12756           
======================================
- Hits         5748    5697   -51     
- Misses       6133    6187   +54     
+ Partials      875     872    -3
Impacted Files Coverage Δ
actor/builtin/storagemarket/storagemarket.go 56% <100%> (+17%) ⬆️
actor/builtin/miner/miner.go 54% <100%> (ø) ⬆️
proofs/sectorbuilder/testing/builder.go 0% <0%> (-88%) ⬇️
proofs/sectorbuilder/testing/harness.go 0% <0%> (-58%) ⬇️
protocol/hello/hello.go 84% <0%> (-6%) ⬇️
node/node.go 59% <0%> (-2%) ⬇️
@laser
laser approved these changes Jun 25, 2019
Copy link
Contributor

left a comment

Nice work, @zen.

@@ -156,10 +156,10 @@ func (sma *Actor) CreateStorageMiner(vmctx exec.VMContext, sectorSize *types.Byt
return ret.(address.Address), 0, nil
}

// UpdatePower is called to reflect a change in the overall power of the network.
// UpdateStorage is called to reflect a change in the overall power of the network.

This comment has been minimized.

Copy link
@laser
@acruikshank
Copy link
Contributor

left a comment

This is great @ZenGround0. Thanks for paying down some debt.
Please add a test for insufficient gas.

@ZenGround0

This comment has been minimized.

Copy link
Contributor Author

commented Jun 25, 2019

Hey @acruikshank I couldn't find any other tests for insufficient gas to draw inspiration from. It turned out to be non-trivial for me to add this in for UpdateStorage given the existing testing framework, especially because the test messages need to come from the miner actor. I decided to not block on this and have added an issue here for testing actors against queries with insufficient gas here: #2995.

UpdateStorage cleanup
- rename UpdatePower to UpdateStorage
- add tests for positive and negative updates
- add test asserting BytesAmount can be negative
- refactor testhelpers

@ZenGround0 ZenGround0 force-pushed the feat/2808 branch from 399761c to be26394 Jun 25, 2019

@ZenGround0 ZenGround0 merged commit bcebaf8 into master Jun 25, 2019

5 checks passed

ci/circleci: build_linux-1 Your tests passed on CircleCI!
Details
ci/circleci: deps_linux Your tests passed on CircleCI!
Details
ci/circleci: functional_test_linux-1 Your tests passed on CircleCI!
Details
ci/circleci: integration_test_linux Your tests passed on CircleCI!
Details
ci/circleci: unit_test_linux Your tests passed on CircleCI!
Details

@ZenGround0 ZenGround0 deleted the feat/2808 branch Jun 25, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.