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

unify workingset api #1702

Merged
merged 1 commit into from
Nov 26, 2019
Merged

unify workingset api #1702

merged 1 commit into from
Nov 26, 2019

Conversation

CoderZhi
Copy link
Collaborator

@CoderZhi CoderZhi commented Nov 22, 2019

  1. Replace UpdateBlockLevelInfo with Finalize
  2. Unify the behavior of RunAction and RunActions
  3. Change version to the height going to build
  4. Verify the block height stored in ctx
  5. Delete all test account creation related code in unit test, and adding those test accounts into Genesis.InitBalanceMap (This change makes the PR very big. It has to be done, because of point 4.) (addressed Delete factory.CreateTestAccount #1668)

Test Plan:
make mockgen && make test && make minicluster

@CoderZhi CoderZhi requested a review from a team as a code owner November 22, 2019 22:26
return stx.runAction(protocol.MustGetRunActionsCtx(ctx), elp)
}

func (stx *stateTX) runAction(

Choose a reason for hiding this comment

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

107-143 lines are duplicate of state/factory/workingset.go:176-213 (from dupl)

return ws.runAction(protocol.MustGetRunActionsCtx(ctx), elp)
}

func (ws *workingSet) runAction(

Choose a reason for hiding this comment

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

176-213 lines are duplicate of state/factory/statetx.go:107-143 (from dupl)

}
}
stx.UpdateBlockLevelInfo(blockHeight)
return receipts, nil
}

// RunAction runs action in the block and track pending changes in working set
func (stx *stateTX) RunAction(
func (stx *stateTX) RunAction(ctx context.Context, elp action.SealedEnvelope) (*action.Receipt, error) {

Choose a reason for hiding this comment

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

hugeParam: elp is heavy (88 bytes); consider passing it by pointer (from gocritic)

func (ws *workingSet) RunAction(
ctx context.Context,
elp action.SealedEnvelope,

Choose a reason for hiding this comment

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

hugeParam: elp is heavy (88 bytes); consider passing it by pointer (from gocritic)

@@ -928,7 +929,7 @@ func (bc *blockchain) pickAndRunActions(ctx context.Context, actionMap map[strin
switch errors.Cause(err) {
case nil:
if !skip {
receipt, err := ws.RunAction(raCtx, putPollResult)
receipt, err := ws.RunAction(ctx, putPollResult)

Choose a reason for hiding this comment

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

shadow: declaration of "err" shadows declaration at line 928 (from govet)

@codecov
Copy link

codecov bot commented Nov 22, 2019

Codecov Report

Merging #1702 into master will decrease coverage by 0.11%.
The diff coverage is 46.15%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1702      +/-   ##
==========================================
- Coverage   55.32%   55.21%   -0.12%     
==========================================
  Files         155      155              
  Lines       13476    13496      +20     
==========================================
- Hits         7456     7452       -4     
- Misses       5061     5071      +10     
- Partials      959      973      +14
Impacted Files Coverage Δ
state/factory/util.go 71.42% <100%> (+62.6%) ⬆️
state/factory/statedb.go 56.44% <31.25%> (-5.44%) ⬇️
state/factory/factory.go 57.81% <33.33%> (-4.3%) ⬇️
blockchain/blockchain.go 59.43% <45.83%> (-0.41%) ⬇️
state/factory/workingset.go 61.7% <50%> (-7.3%) ⬇️
state/factory/statetx.go 53.84% <52.38%> (-7.98%) ⬇️
db/trie/branchnode.go 67.2% <0%> (+1.6%) ⬆️
db/trie/extensionnode.go 59.32% <0%> (+1.69%) ⬆️
... and 1 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 abdff29...f2ff65b. Read the comment docs.

// Finalize runs action in the block and track pending changes in working set
func (stx *stateTX) Finalize() error {
if stx.finalized {
return errors.New("Cannot finalize a working set twice")

Choose a reason for hiding this comment

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

error strings should not be capitalized or end with punctuation or a newline (from golint)

// Finalize runs action in the block and track pending changes in working set
func (ws *workingSet) Finalize() error {
if ws.finalized {
return errors.New("Cannot finalize a working set twice")

Choose a reason for hiding this comment

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

error strings should not be capitalized or end with punctuation or a newline (from golint)


func (ws *workingSet) runAction(
ctx context.Context,
elp action.SealedEnvelope,

Choose a reason for hiding this comment

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

hugeParam: elp is heavy (88 bytes); consider passing it by pointer (from gocritic)

if err != nil {
return err
}
if err = blk.VerifyDeltaStateDigest(digest); err != nil {

Choose a reason for hiding this comment

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

sloppyReassign: re-assignment to err can be replaced with err := blk.VerifyDeltaStateDigest(digest) (from gocritic)

@@ -259,32 +246,41 @@ func testState(sf Factory, t *testing.T) {
registry := protocol.NewRegistry()
acc := account.NewProtocol()
require.NoError(t, registry.Register(account.ProtocolID, acc))
require.NoError(t, sf.Start(context.Background()))
ge := genesis.Default
ge.InitBalanceMap[a] = "100"

Choose a reason for hiding this comment

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

string 100 has 2 occurrences, make it a constant (from goconst)

@@ -84,19 +83,19 @@ func TestActPool_NewActPool(t *testing.T) {

func TestActPool_validateGenericAction(t *testing.T) {
require := require.New(t)
cfg := config.Default
cfg.Genesis.InitBalanceMap[addr1] = "100"

Choose a reason for hiding this comment

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

string 100 has 13 occurrences, make it a constant (from goconst)

require.NoError(registry.Register(account.ProtocolID, account.NewProtocol()))
cfg := config.Default
cfg.Genesis.InitBalanceMap[addr1] = "100"
cfg.Genesis.InitBalanceMap[addr2] = "10"

Choose a reason for hiding this comment

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

string 10 has 3 occurrences, make it a constant (from goconst)

@CoderZhi CoderZhi merged commit a5b2587 into master Nov 26, 2019
@CoderZhi CoderZhi deleted the change_run_action_parameter branch November 26, 2019 02:10
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