Move state factory related code from blockchain to sf #1642

koseoyoung · 2019-11-12T18:13:59Z

move initialize() from blockchain.go to state factory
handle dependency loop issue in unit tests

golangcibot · 2019-11-12T18:17:06Z

state/factory/factory.go

@@ -399,3 +401,24 @@ func (sf *factory) accountState(encodedAddr string) (*state.Account, error) {
 	}
 	return &account, nil
 }
+
+//Initialize initializes the state factory
+func (sf *factory) Initialize(cfg config.Config, registry *protocol.Registry) error {


406-424 lines are duplicate of state/factory/statedb.go:287-305 (from dupl)

golangcibot · 2019-11-12T18:17:06Z

state/factory/statedb.go

@@ -282,3 +282,24 @@ func (sdb *stateDB) accountState(encodedAddr string) (*state.Account, error) {
 	}
 	return &account, nil
 }
+
+//Initialize initializes the state db
+func (sdb *stateDB) Initialize(cfg config.Config, registry *protocol.Registry) error {


287-305 lines are duplicate of state/factory/factory.go:406-424 (from dupl)

golangcibot · 2019-11-12T18:17:07Z

state/factory/util.go

+	return createRewardingGenesisStates(ctx, ws, registry, cfg)
+}
+
+func createAccountGenesisStates(ctx context.Context, ws WorkingSet, registry *protocol.Registry, cfg config.Config) error {


ws can be github.com/iotexproject/iotex-core/action/protocol.StateManager (from interfacer)

golangcibot · 2019-11-12T18:17:07Z

state/factory/util.go

+	return ap.Initialize(ctx, ws, addrs, balances)
+}
+
+func createRewardingGenesisStates(ctx context.Context, ws WorkingSet, registry *protocol.Registry, cfg config.Config) error {


ws can be github.com/iotexproject/iotex-core/action/protocol.StateManager (from interfacer)

golangcibot · 2019-11-12T18:17:07Z

state/factory/util.go

+	)
+}
+
+func createPollGenesisStates(ctx context.Context, ws WorkingSet, registry *protocol.Registry, cfg config.Config) error {


ws can be github.com/iotexproject/iotex-core/action/protocol.StateManager (from interfacer)

golangcibot · 2019-11-12T18:17:08Z

state/factory/factory.go

@@ -399,3 +401,24 @@ func (sf *factory) accountState(encodedAddr string) (*state.Account, error) {
 	}
 	return &account, nil
 }
+
+//Initialize initializes the state factory


commentFormatting: put a space between // and comment text (from gocritic)

golangcibot · 2019-11-12T18:17:08Z

state/factory/statedb.go

@@ -282,3 +282,24 @@ func (sdb *stateDB) accountState(encodedAddr string) (*state.Account, error) {
 	}
 	return &account, nil
 }
+
+//Initialize initializes the state db


commentFormatting: put a space between // and comment text (from gocritic)

golangcibot · 2019-11-12T18:17:08Z

state/factory/util.go

+	"github.com/pkg/errors"
+)
+
+//CreateGenesisStates initialize the states


commentFormatting: put a space between // and comment text (from gocritic)

golangcibot · 2019-11-12T18:17:09Z

state/factory/util.go

+)
+
+//CreateGenesisStates initialize the states
+func CreateGenesisStates(cfg config.Config, registry *protocol.Registry, ws WorkingSet) error {


hugeParam: cfg is heavy (1488 bytes); consider passing it by pointer (from gocritic)

golangcibot · 2019-11-12T18:17:09Z

state/factory/util.go

+	"go.uber.org/zap"
+
+	"github.com/iotexproject/go-pkgs/hash"
+	"github.com/iotexproject/iotex-address/address"


File is not goimports-ed with -local github.com/iotexproject/iotex-core (from goimports)

Suggested change

"github.com/iotexproject/iotex-address/address"

"github.com/iotexproject/iotex-address/address"

"github.com/iotexproject/iotex-proto/golang/iotextypes"

"github.com/pkg/errors"

codecov · 2019-11-12T22:55:35Z

Codecov Report

Merging #1642 into master will decrease coverage by 0.49%.
The diff coverage is 8.58%.

@@            Coverage Diff            @@
##           master    #1642     +/-   ##
=========================================
- Coverage   54.72%   54.23%   -0.5%     
=========================================
  Files         154      155      +1     
  Lines       13459    13480     +21     
=========================================
- Hits         7366     7311     -55     
- Misses       5138     5224     +86     
+ Partials      955      945     -10

Impacted Files	Coverage Δ
state/factory/util.go	`0% <0%> (ø)`
action/protocol/execution/evm/evmstatedbadapter.go	`52.57% <0%> (-0.24%)`	⬇️
state/factory/factory.go	`54.5% <22.58%> (-2.86%)`	⬇️
state/factory/statedb.go	`61.53% <24%> (-3.29%)`	⬇️
blockchain/blockchain.go	`41.98% <33.33%> (-1.23%)`	⬇️
action/protocol/poll/protocol.go	`35.75% <0%> (-1.27%)`	⬇️

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 db4a207...e7169a8. Read the comment docs.

golangcibot · 2019-11-12T23:10:50Z

state/factory/util.go

+)
+
+// CreateGenesisStates initialize the states
+func CreateGenesisStates(cfg config.Config, registry *protocol.Registry, sm protocol.StateManager) error {


hugeParam: cfg is heavy (1488 bytes); consider passing it by pointer (from gocritic)

golangcibot · 2019-11-12T23:10:51Z

state/factory/util.go

+	return createRewardingGenesisStates(ctx, sm, registry, cfg)
+}
+
+func createAccountGenesisStates(ctx context.Context, sm protocol.StateManager, registry *protocol.Registry, cfg config.Config) error {


hugeParam: cfg is heavy (1488 bytes); consider passing it by pointer (from gocritic)

golangcibot · 2019-11-12T23:10:51Z

state/factory/util.go

+	return ap.Initialize(ctx, sm, addrs, balances)
+}
+
+func createRewardingGenesisStates(ctx context.Context, sm protocol.StateManager, registry *protocol.Registry, cfg config.Config) error {


hugeParam: cfg is heavy (1488 bytes); consider passing it by pointer (from gocritic)

CoderZhi

quite a few comments offline, please address

CoderZhi · 2019-11-13T00:47:48Z

action/protocol/rewarding/admin_test.go

-		ws, err := stateDB.NewWorkingSet()
-		require.NoError(t, err)
-		amount, err := p.EpochReward(ctx, ws)
+	testProtocol(t, func(t *testing.T, ctx context.Context, sm *mock_chainmanager.MockStateManager, p *Protocol) {


StateManager

CoderZhi · 2019-11-13T00:48:04Z

action/protocol/poll/protocol_test.go

 	"github.com/iotexproject/iotex-core/test/identityset"
 	"github.com/iotexproject/iotex-core/test/mock/mock_chainmanager"
 	"github.com/iotexproject/iotex-election/test/mock/mock_committee"
 	"github.com/iotexproject/iotex-election/types"
 )

-func initConstruct(t *testing.T) (Protocol, context.Context, factory.WorkingSet, *types.ElectionResult) {
+func initConstruct(t *testing.T) (Protocol, context.Context, *mock_chainmanager.MockStateManager, *types.ElectionResult) {


StateManager

CoderZhi · 2019-11-13T00:48:21Z

action/protocol/rewarding/fund_test.go

 )

 func TestProtocol_Fund(t *testing.T) {
-	testProtocol(t, func(t *testing.T, ctx context.Context, stateDB factory.Factory, p *Protocol) {
+	testProtocol(t, func(t *testing.T, ctx context.Context, sm *mock_chainmanager.MockStateManager, p *Protocol) {


StateManager

golangcibot · 2019-11-13T05:01:58Z

state/factory/util.go

+)
+
+// initializeFactory initializes the factory
+func initializeFactory(sf Factory, cfg config.Config, registry *protocol.Registry) error {


hugeParam: cfg is heavy (1488 bytes); consider passing it by pointer (from gocritic)

CoderZhi

looks good at first glance. It is a large PR, let me take a chance to look through everything in details.

CoderZhi · 2019-11-13T17:10:40Z

action/protocol/execution/evm/evmstatedbadapter_test.go

 )

+func initMockStateManager(t *testing.T) protocol.StateManager {


func initMockStateManager(ctrl gomock.Controller) protocol.StateManager {

Move

ctrl := gomock.NewController(t) defer ctrl.Finish()

out of the function

CoderZhi · 2019-11-13T17:13:31Z

action/protocol/poll/protocol_test.go

 	"github.com/iotexproject/iotex-core/test/identityset"
 	"github.com/iotexproject/iotex-core/test/mock/mock_chainmanager"
 	"github.com/iotexproject/iotex-election/test/mock/mock_committee"
 	"github.com/iotexproject/iotex-election/types"
 )

-func initConstruct(t *testing.T) (Protocol, context.Context, factory.WorkingSet, *types.ElectionResult) {
+func initConstruct(t *testing.T) (Protocol, context.Context, protocol.StateManager, *types.ElectionResult) {
 	require := require.New(t)
 	ctrl := gomock.NewController(t)


same here, move it out

CoderZhi · 2019-11-13T17:13:58Z

action/protocol/poll/protocol_test.go

 	"github.com/iotexproject/iotex-core/test/identityset"
 	"github.com/iotexproject/iotex-core/test/mock/mock_chainmanager"
 	"github.com/iotexproject/iotex-election/test/mock/mock_committee"
 	"github.com/iotexproject/iotex-election/types"
 )

-func initConstruct(t *testing.T) (Protocol, context.Context, factory.WorkingSet, *types.ElectionResult) {
+func initConstruct(t *testing.T) (Protocol, context.Context, protocol.StateManager, *types.ElectionResult) {


return (Protocol, context.Context, protocol.StateManager, *types.ElectionResult, error)

CoderZhi · 2019-11-13T17:21:34Z

blockchain/blockchain.go

@@ -694,23 +691,7 @@ func (bc *blockchain) blockFooterByHeight(height uint64) (*block.Footer, error)
 }

 func (bc *blockchain) startEmptyBlockchain() error {


this function is only used once. We may delete it

CoderZhi · 2019-11-13T17:23:51Z

state/factory/factory.go

@@ -64,6 +64,8 @@ type (

 		State(hash.Hash160, interface{}) error
 		AddActionHandlers(...protocol.ActionHandler)
+		// empty blockchain
+		Initialize(config.Config, *protocol.Registry) error


the name of this interface is not accurate. could we come up with a better one?

CoderZhi · 2019-11-13T17:24:29Z

state/factory/factory.go

+
+// Initialize initializes the state factory
+func (sf *factory) Initialize(cfg config.Config, registry *protocol.Registry) error {
+	return initializeFactory(sf, cfg, registry)


as discussed offline, we need to call

sf.mu.Lock() defer sf.mu.Unlock()

golangcibot · 2019-11-13T19:34:55Z

state/factory/statedb.go

+}
+
+// Initialize initializes the state db
+func (sdb *stateDB) Initialize(cfg config.Config, registry *protocol.Registry) error {


hugeParam: cfg is heavy (1488 bytes); consider passing it by pointer (from gocritic)

golangcibot · 2019-11-13T19:34:55Z

state/factory/util.go

+)
+
+// createGenesisStates initialize the genesis states
+func createGenesisStates(cfg config.Config, registry *protocol.Registry, ws WorkingSet) error {


hugeParam: cfg is heavy (1488 bytes); consider passing it by pointer (from gocritic)

golangcibot · 2019-11-13T19:34:55Z

blockchain/blockchain.go

@@ -315,8 +312,9 @@ func (bc *blockchain) Start(ctx context.Context) (err error) {
 	if bc.tipHeight, err = bc.dao.GetTipHeight(); err != nil {
 		return err
 	}
+	//start empty blockchain


commentFormatting: put a space between // and comment text (from gocritic)

koseoyoung requested a review from CoderZhi November 12, 2019 18:14

golangcibot reviewed Nov 12, 2019

View reviewed changes

koseoyoung marked this pull request as ready for review November 12, 2019 22:44

koseoyoung requested a review from a team as a code owner November 12, 2019 22:44

golangcibot reviewed Nov 12, 2019

View reviewed changes

CoderZhi reviewed Nov 13, 2019

View reviewed changes

golangcibot reviewed Nov 13, 2019

View reviewed changes

CoderZhi reviewed Nov 13, 2019

View reviewed changes

koseoyoung and others added 9 commits November 13, 2019 11:31

move startEmptyBlockchain into sf Initialize

244dd14

change test files

14547d8

change test files

55557e7

mock state snapshot/state/revert/putstate

a05b91d

refactor

d144fc7

change to state manager interface/ refactor

ac73717

refactor/add comments

4c7ce20

wrap up comment

7259f2d

refactor unit tests and bc/sf

e7169a8

koseoyoung force-pushed the refactor_bc branch from a390ccb to e7169a8 Compare November 13, 2019 19:31

golangcibot reviewed Nov 13, 2019

View reviewed changes

CoderZhi approved these changes Nov 13, 2019

View reviewed changes

koseoyoung merged commit 56912d6 into iotexproject:master Nov 13, 2019

koseoyoung mentioned this pull request Nov 13, 2019

Add e2e test using state factory #1646

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move state factory related code from blockchain to sf #1642

Move state factory related code from blockchain to sf #1642

koseoyoung commented Nov 12, 2019 •

edited

golangcibot Nov 12, 2019

golangcibot Nov 12, 2019

golangcibot Nov 12, 2019

golangcibot Nov 12, 2019

golangcibot Nov 12, 2019

golangcibot Nov 12, 2019

golangcibot Nov 12, 2019

golangcibot Nov 12, 2019

golangcibot Nov 12, 2019

golangcibot Nov 12, 2019

codecov bot commented Nov 12, 2019 •

edited

golangcibot Nov 12, 2019

golangcibot Nov 12, 2019

golangcibot Nov 12, 2019

CoderZhi left a comment

CoderZhi Nov 13, 2019

CoderZhi Nov 13, 2019

CoderZhi Nov 13, 2019

golangcibot Nov 13, 2019

CoderZhi left a comment

CoderZhi Nov 13, 2019

CoderZhi Nov 13, 2019

CoderZhi Nov 13, 2019

CoderZhi Nov 13, 2019

CoderZhi Nov 13, 2019

CoderZhi Nov 13, 2019

golangcibot Nov 13, 2019

golangcibot Nov 13, 2019

golangcibot Nov 13, 2019

		)

		func initMockStateManager(t *testing.T) protocol.StateManager {

		@@ -694,23 +691,7 @@ func (bc blockchain) blockFooterByHeight(height uint64) (block.Footer, error)
		}

		func (bc *blockchain) startEmptyBlockchain() error {

Move state factory related code from blockchain to sf #1642

Move state factory related code from blockchain to sf #1642

Conversation

koseoyoung commented Nov 12, 2019 • edited

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

codecov bot commented Nov 12, 2019 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CoderZhi 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

CoderZhi 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

koseoyoung commented Nov 12, 2019 •

edited

codecov bot commented Nov 12, 2019 •

edited