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

Cleanup SCRs which is informational #3600

Merged
merged 28 commits into from
Nov 26, 2021

Conversation

sasurobert
Copy link
Contributor

Cleanup SCRs which is informational

@codecov
Copy link

codecov bot commented Nov 23, 2021

Codecov Report

Merging #3600 (7fbc455) into feat/SC-proc-improvements-NOV (82091ce) will increase coverage by 0.00%.
The diff coverage is 81.48%.

❗ Current head 7fbc455 differs from pull request most recent head 1cb8fd4. Consider uploading reports for the commit 1cb8fd4 to get more accurate results
Impacted file tree graph

@@                      Coverage Diff                       @@
##           feat/SC-proc-improvements-NOV    #3600   +/-   ##
==============================================================
  Coverage                          74.00%   74.00%           
==============================================================
  Files                                582      582           
  Lines                              74910    74980   +70     
==============================================================
+ Hits                               55436    55488   +52     
- Misses                             15067    15088   +21     
+ Partials                            4407     4404    -3     
Impacted Files Coverage Δ
node/nodeRunner.go 0.00% <0.00%> (ø)
process/smartContract/testScProcessor.go 11.66% <0.00%> (-35.51%) ⬇️
process/smartContract/process.go 68.37% <87.50%> (+1.41%) ⬆️
factory/blockProcessorCreator.go 79.32% <100.00%> (ø)
genesis/process/metaGenesisBlockCreator.go 49.59% <100.00%> (ø)
genesis/process/shardGenesisBlockCreator.go 42.97% <100.00%> (ø)
process/smartContract/hooks/blockChainHook.go 57.84% <0.00%> (-0.67%) ⬇️
... and 5 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 21aa236...1cb8fd4. Read the comment docs.

config/tomlConfig_test.go Outdated Show resolved Hide resolved
process/smartContract/process.go Show resolved Hide resolved
process/smartContract/process.go Outdated Show resolved Hide resolved
process/smartContract/process.go Outdated Show resolved Hide resolved
process/smartContract/process.go Outdated Show resolved Hide resolved
process/smartContract/process.go Outdated Show resolved Hide resolved
process/smartContract/process.go Outdated Show resolved Hide resolved
process/smartContract/process.go Outdated Show resolved Hide resolved
config/epochConfig.go Outdated Show resolved Hide resolved
node/nodeRunner.go Outdated Show resolved Hide resolved
return true
}

if core.IsSmartContractAddress(scr.GetRcvAddr()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this if could be put above L465

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope. because you can have cross shard built in function call to a user

cleanedUPSCrs = append(cleanedUPSCrs, scr)
}

if sc.flagCleanUpSCRData.IsSet() {
Copy link
Contributor

Choose a reason for hiding this comment

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

if !sc.flagCleanUpSCRData.IsSet()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -619,8 +619,8 @@ func TestEnableEpochConfig(t *testing.T) {
# CreateNFTThroughExecByCallerEnableEpoch represents the epoch when nft creation through execution on destination by caller is enabled
CreateNFTThroughExecByCallerEnableEpoch = 47

# CleanUpSCRDataEnableEpoch represents the epoch the informational scrs are cleaned from miniblocks
CleanUpSCRDataEnableEpoch = 48
# CleanUpInformativeSCRsEnableEpoch represents the epoch when the scrs which contain only information are cleaned from miniblocks and logs are created from it
Copy link
Contributor

Choose a reason for hiding this comment

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

# CleanUpInformativeSCRsEnableEpoch represents the epoch when the informative-only scrs are cleaned from miniblocks and logs are created from them

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


return ti
}

func (ti *testIndexer) SetTxLogProcessor(txsLogsProcessor process.TransactionLogProcessor) {
Copy link
Contributor

Choose a reason for hiding this comment

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

add mock comment?

Copy link
Contributor

Choose a reason for hiding this comment

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

also, why do we need a setter function? in almost every case I've seen in this PR, you call this setter right after the constructor. Might move the setting of txsLogProcessor in the constructor and only call this setter when something custom is needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -753,6 +756,15 @@ func createDefaultVMConfig() *config.VirtualMachineConfig {
}
}

type ResultsCreateTxProcessor struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

add mock comment? also, please rename to CreateTxProcessorResults

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@sasurobert sasurobert changed the base branch from development to feat/SC-proc-improvements-NOV November 26, 2021 07:15
bogdan-rosianu
bogdan-rosianu previously approved these changes Nov 26, 2021
…ational

# Conflicts:
#	cmd/node/config/enableEpochs.toml
#	config/epochConfig.go
#	config/tomlConfig_test.go
#	node/nodeRunner.go
Copy link
Collaborator

@gabi-vuls gabi-vuls left a comment

Choose a reason for hiding this comment

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

System test passed.
image

@sasurobert sasurobert merged commit a7ad92d into feat/SC-proc-improvements-NOV Nov 26, 2021
@sasurobert sasurobert deleted the cleanup-SCRs-informational branch November 26, 2021 10:53
sc.flagCleanUpSCRData.Toggle(epoch >= sc.cleanupSCRDataEnableEpoch)
log.Debug("scProcessor: cleanup scr data", "enabled", sc.flagCleanUpSCRData.IsSet())
sc.flagCleanUpInformativeSCRs.Toggle(epoch >= sc.cleanUpInformativeSCRsEnableEpoch)
log.Debug("scProcessor: cleanup scr data", "enabled", sc.flagCleanUpInformativeSCRs.IsSet())
Copy link
Contributor

Choose a reason for hiding this comment

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

cleanup informative SCRs

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.

None yet

5 participants