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: moved the blockchain instantiation from the vm container factory #3825

Merged
merged 3 commits into from
Feb 23, 2022

Conversation

iulianpascalau
Copy link
Contributor

@iulianpascalau iulianpascalau commented Feb 21, 2022

  • moved the blockchain hook instantiation from the vm container factory
  • switched the usage of the accounts adapter API on the api-related operations

@codecov
Copy link

codecov bot commented Feb 21, 2022

Codecov Report

Merging #3825 (7d1f98d) into development (9e73e17) will decrease coverage by 0.00%.
The diff coverage is 67.21%.

Impacted file tree graph

@@               Coverage Diff               @@
##           development    #3825      +/-   ##
===============================================
- Coverage        74.62%   74.61%   -0.01%     
===============================================
  Files              606      606              
  Lines            79672    79694      +22     
===============================================
+ Hits             59457    59467      +10     
- Misses           15640    15648       +8     
- Partials          4575     4579       +4     
Impacted Files Coverage Δ
heartbeat/process/monitor.go 83.37% <ø> (-0.10%) ⬇️
genesis/process/shardGenesisBlockCreator.go 39.74% <37.50%> (+0.07%) ⬆️
factory/apiResolverFactory.go 75.47% <50.00%> (-1.60%) ⬇️
genesis/process/metaGenesisBlockCreator.go 47.91% <60.00%> (+0.02%) ⬆️
factory/blockProcessorCreator.go 80.05% <66.66%> (-0.31%) ⬇️
process/factory/metachain/vmContainerFactory.go 70.00% <87.50%> (-0.23%) ⬇️
process/factory/shard/vmContainerFactory.go 76.75% <92.85%> (+2.30%) ⬆️
p2p/libp2p/netMessenger.go 78.86% <0.00%> (-0.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 07da6ef...7d1f98d. Read the comment docs.

andreibancioiu
andreibancioiu previously approved these changes Feb 21, 2022
@@ -59,6 +58,8 @@ type ArgsNewVMContainerFactory struct {
EpochNotifier process.EpochNotifier
EpochConfig *config.EpochConfig
ShardCoordinator sharding.Coordinator
PubkeyConv core.PubkeyConverter
BlockChainHook process.BlockChainHookHandler
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

@@ -230,8 +230,6 @@ func (m *Monitor) loadHeartbeatsFromStorer(pubKey string) (*heartbeatMessageInfo
receivedHbmi := m.convertFromExportedStruct(*heartbeatDTO, m.maxDurationPeerUnresponsive)
receivedHbmi.getTimeHandler = m.timer.Now
crtTime := m.timer.Now()
crtDuration := crtTime.Sub(receivedHbmi.lastUptimeDowntime)
crtDuration = maxDuration(0, crtDuration)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unused variable not spotted by the compiler? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

spotted by the linter, actually. :)

// IsPayable -
func (stub *BlockChainHookStub) IsPayable(_ []byte, recvAddress []byte) (bool, error) {
if stub.IsPayableCalled != nil {
return stub.IsPayableCalled(recvAddress)
Copy link
Contributor

Choose a reason for hiding this comment

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

don't ignore the first param as we might have some tests where we'd like to check it

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

argsNewVmFactory := metachain.ArgsNewVMContainerFactory{
ArgBlockChainHook: argsHook,
BlockChainHook: blockChainHookImpl,
Copy link
Contributor

Choose a reason for hiding this comment

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

CreateApiResolver uses AccountsAdapter and not AccountsAdapterAPI. expected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

switched the usage of the AccountsAdapterApi but kept the read only accounts adapter wrapper in order to keep the consistency when executing (readonly accounts adapter won't call SaveAccount, for example, but won't return an error as the AccountsAdapterApi would)

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.

@gabi-vuls gabi-vuls merged commit e85dc8e into development Feb 23, 2022
@gabi-vuls gabi-vuls deleted the extract-blockchainHook-instantiation branch February 23, 2022 17:06
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

4 participants