Mx 15315 refactor sov runtype comp #6065

axenteoctavian · 2024-03-25T16:53:31Z

Reasoning behind the pull request

Refactor RunType components for Chain Simulator
refactored DataCodec and TopicsChecker

Proposed changes

Refactor RunType components

Testing procedure

Pre-requisites

Based on the Contributing Guidelines the PR author and the reviewers must check the following requirements are met:

was the PR targeted to the correct branch?
if this is a larger feature that probably needs more than one PR, is there a feat branch created?
if this is a feat branch merging, do all satellite projects have a proper tag inside go.mod?

…r-sov-runtype-comp # Conflicts: # testscommon/components/components.go

codecov · 2024-04-01T09:21:26Z

Codecov Report

Attention: Patch coverage is 83.33333% with 12 lines in your changes are missing coverage. Please review.

Project coverage is 78.17%. Comparing base (19ae2f2) to head (b0dea1e).
Report is 1 commits behind head on feat/refactor-runtype-components.

❗ Current head b0dea1e differs from pull request most recent head 9569ac8. Consider uploading reports for the commit 9569ac8 to get more accurate results

Files	Patch %	Lines
factory/runType/runTypeComponentsHandler.go	50.00%	6 Missing and 6 partials ⚠️

Additional details and impacted files

@@                         Coverage Diff                          @@
##           feat/refactor-runtype-components    #6065      +/-   ##
====================================================================
+ Coverage                             78.11%   78.17%   +0.05%     
====================================================================
  Files                                   874      874              
  Lines                                103470   103512      +42     
====================================================================
+ Hits                                  80828    80917      +89     
+ Misses                                17153    17106      -47     
  Partials                               5489     5489

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

cmd/sovereignnode/sovereignNodeRunner.go

factory/processing/processComponents.go

factory/runType/runTypeComponents.go

factory/runType/runTypeComponentsHandler.go

cmd/sovereignnode/sovereignNodeRunner.go

mariusmihaic · 2024-04-09T09:09:25Z

factory/runType/runTypeComponentsHandler_test.go

+		require.NotNil(t, managedRunTypeComponents.AdditionalStorageServiceCreator())
+		require.NotNil(t, managedRunTypeComponents.SCProcessorCreator())
+		require.NotNil(t, managedRunTypeComponents.SCResultsPreProcessorCreator())
+		require.NotEqual(t, consensus.ConsensusModelInvalid, managedRunTypeComponents.ConsensusModel())


Can we check against the expected value (v1/v2)?

ConsensusModelInvalid is default

There you check that the consensus model IS NOT invalid.
Therefore it should be v1/v2. Please check against the values accordingly

factory/runType/sovereignRunTypeComponents.go

process/block/baseProcess_test.go

factory/processing/processComponents.go

raduchis · 2024-04-10T13:26:44Z

factory/runType/runTypeComponentsHandler.go

@@ -131,6 +133,24 @@ func (mrc *managedRunTypeComponents) CheckSubcomponents() error {
 	if check.IfNil(mrc.scResultPreProcessorCreator) {
 		return errors.ErrNilSCResultsPreProcessorCreator
 	}
+	if mrc.consensusModel == consensus.ConsensusModelInvalid {
+		return errors.ErrInvalidConsensusModel


please add unit tests for the newly added checks.

as discussed, I added a TODO

raduchis · 2024-04-10T13:28:26Z

factory/runType/runTypeComponentsHandler.go

@@ -362,6 +382,30 @@ func (mrc *managedRunTypeComponents) AccountsCreator() state.AccountFactory {
 	return mrc.runTypeComponents.accountsCreator
 }

+// DataCodecHandler returns the data codec factory
+func (mrc *managedRunTypeComponents) DataCodecHandler() sovereign.DataDecoderHandler {


the name of the method and the return are different.
Could DataCodecHandler() be renamed to DataDecoderHandler()?

renamed everywhere to DataCodec and DataCodecHandler because it does both decoding and encoding and the name was misleading

raduchis · 2024-04-10T13:29:39Z

factory/runType/runTypeComponentsHandler.go

@@ -362,6 +382,30 @@ func (mrc *managedRunTypeComponents) AccountsCreator() state.AccountFactory {
 	return mrc.runTypeComponents.accountsCreator
 }

+// DataCodecHandler returns the data codec factory
+func (mrc *managedRunTypeComponents) DataCodecHandler() sovereign.DataDecoderHandler {
+	mrc.mutStateComponents.RLock()


the mutex has is called with StateComponents. I think it was my bad naming. Please rename it to mutRunTypeComponents.

cmd/sovereignnode/sovereignNodeRunner.go

factory/processing/processComponents.go

mariusmihaic · 2024-04-15T08:46:00Z

factory/runType/runTypeComponentsHandler_test.go

+		require.NotNil(t, managedRunTypeComponents.AdditionalStorageServiceCreator())
+		require.NotNil(t, managedRunTypeComponents.SCProcessorCreator())
+		require.NotNil(t, managedRunTypeComponents.SCResultsPreProcessorCreator())
+		require.NotEqual(t, consensus.ConsensusModelInvalid, managedRunTypeComponents.ConsensusModel())


There you check that the consensus model IS NOT invalid.
Therefore it should be v1/v2. Please check against the values accordingly

process/block/baseProcess_test.go

mariusmihaic · 2024-04-16T09:17:19Z

factory/runType/runTypeComponentsHandler_test.go

@@ -85,11 +85,11 @@ func TestManagedRunTypeComponents_Create(t *testing.T) {
 		require.NotNil(t, managedRunTypeComponents.AdditionalStorageServiceCreator())
 		require.NotNil(t, managedRunTypeComponents.SCProcessorCreator())
 		require.NotNil(t, managedRunTypeComponents.SCResultsPreProcessorCreator())
-		require.NotEqual(t, consensus.ConsensusModelInvalid, managedRunTypeComponents.ConsensusModel())
+		require.Equal(t, consensus.ConsensusModelV1, managedRunTypeComponents.ConsensusModel())


Not necessarily in this PR, but we should have a similar unit test for sovereign run type comps

axenteoctavian added 2 commits March 25, 2024 16:26

interfaces renaming

8a3da80

factories added

010790e

axenteoctavian marked this pull request as draft March 25, 2024 16:53

axenteoctavian added 10 commits March 25, 2024 19:07

resolve cyclic imports

acfef0d

fix tests

725f2ca

fix runtype components tests

1ab6435

new unit tests, runtype tests fixes

d267d76

fix unit tests

2a28037

fix unit tests

bc66e6d

import fixes

a710cd7

moved interfaces

7651b29

fixed imports

21aad66

fixed imports

9f397cd

axenteoctavian marked this pull request as ready for review March 26, 2024 12:33

axenteoctavian added 7 commits March 26, 2024 15:20

fix imports

99b4166

deleted factories, using only component

6bb322e

errors fixes

0ebba8a

fixes for imports

4f36041

nil checks and unit tests

c57bbad

Merge branch 'feat/refactor-runtype-components' into MX-15315-refacto…

d6bcad4

…r-sov-runtype-comp # Conflicts: # testscommon/components/components.go

fixes after merge

cf7fa5c

mariusmihaic self-requested a review April 9, 2024 08:49

mariusmihaic reviewed Apr 9, 2024

View reviewed changes

axenteoctavian added 5 commits April 9, 2024 17:16

fixes after review

00a9293

fixes after review

58fea75

tests fixes

74fdb53

components fixes

0a6dad1

components fixes

b0dea1e

raduchis self-requested a review April 10, 2024 13:04

raduchis requested changes Apr 10, 2024

View reviewed changes

axenteoctavian added 2 commits April 10, 2024 18:01

renamings and stuff

a913adb

consensul model TODO

01443e4

mariusmihaic reviewed Apr 15, 2024

View reviewed changes

mariusmihaic requested changes Apr 15, 2024

View reviewed changes

process/block/baseProcess_test.go Outdated Show resolved Hide resolved

axenteoctavian added 3 commits April 15, 2024 13:28

fixes after review

74a3d3f

fixes after review, renamed everywhere to DataCodec

2f96d57

TODO Added for RunType subcomponents

9569ac8

mariusmihaic approved these changes Apr 16, 2024

View reviewed changes

raduchis approved these changes Apr 16, 2024

View reviewed changes

axenteoctavian merged commit 462bf36 into feat/refactor-runtype-components Apr 16, 2024
4 checks passed

axenteoctavian deleted the MX-15315-refactor-sov-runtype-comp branch April 16, 2024 13:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mx 15315 refactor sov runtype comp #6065

Mx 15315 refactor sov runtype comp #6065

axenteoctavian commented Mar 25, 2024 •

edited

Loading

codecov bot commented Apr 1, 2024 •

edited

Loading

mariusmihaic Apr 9, 2024

axenteoctavian Apr 9, 2024

mariusmihaic Apr 15, 2024

raduchis Apr 10, 2024

axenteoctavian Apr 16, 2024 •

edited

Loading

raduchis Apr 10, 2024

axenteoctavian Apr 15, 2024

raduchis Apr 10, 2024

mariusmihaic Apr 15, 2024

mariusmihaic Apr 16, 2024

Mx 15315 refactor sov runtype comp #6065

Mx 15315 refactor sov runtype comp #6065

Conversation

axenteoctavian commented Mar 25, 2024 • edited Loading

Reasoning behind the pull request

Proposed changes

Testing procedure

Pre-requisites

codecov bot commented Apr 1, 2024 • edited Loading

Codecov Report

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

axenteoctavian Apr 16, 2024 • edited Loading

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

axenteoctavian commented Mar 25, 2024 •

edited

Loading

codecov bot commented Apr 1, 2024 •

edited

Loading

axenteoctavian Apr 16, 2024 •

edited

Loading