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

refactor(core-blockchain): initialize blockchain when resolved first time #3969

Merged
merged 5 commits into from Aug 21, 2020

Conversation

rainydio
Copy link
Contributor

@rainydio rainydio commented Aug 20, 2020

Summary

Blockchain class was resolved during register, which chain-resolved wallet repository and all wallet indices which were registered, any wallet index that is registered afterwards wasn't available.

The reason why it was resolved is because Blockchain class needed additional constructor logic to setup state machine. I marked initialize function with @postConstruct decorator and removed direct resolve and initialize call from register.

Now plugin that adds wallet index (e.g. core-magistrate-transactions) doesn't have to be registered before core-blockchain.

Checklist

  • Tests
  • Ready to be merged

@codecov
Copy link

codecov bot commented Aug 20, 2020

Codecov Report

Merging #3969 into develop will increase coverage by 1.75%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #3969      +/-   ##
===========================================
+ Coverage    95.69%   97.44%   +1.75%     
===========================================
  Files          631      631              
  Lines        14583    14582       -1     
  Branches      1733     1734       +1     
===========================================
+ Hits         13955    14210     +255     
+ Misses         437      181     -256     
  Partials       191      191              
Flag Coverage Δ
#functional 6.80% <0.00%> (?)
#integration 9.88% <0.00%> (+<0.01%) ⬆️
#unit 95.61% <100.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
packages/core-blockchain/src/blockchain.ts 96.88% <100.00%> (+0.04%) ⬆️
packages/core-blockchain/src/service-provider.ts 100.00% <100.00%> (ø)
...re-test-framework/src/utils/transaction-factory.ts 98.86% <0.00%> (+2.84%) ⬆️
packages/core-database/src/utils/transform.ts 100.00% <0.00%> (+25.00%) ⬆️
...core-database/src/repositories/round-repository.ts 87.50% <0.00%> (+50.00%) ⬆️
...-framework/src/matchers/functional/vote-balance.ts 90.00% <0.00%> (+70.00%) ⬆️
...atabase/src/repositories/transaction-repository.ts 100.00% <0.00%> (+78.37%) ⬆️
...core-database/src/repositories/block-repository.ts 89.04% <0.00%> (+80.82%) ⬆️
...e-test-framework/src/matchers/functional/entity.ts 93.54% <0.00%> (+87.09%) ⬆️
...e-database/src/repositories/abstract-repository.ts 96.66% <0.00%> (+90.00%) ⬆️
... and 23 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 af5e542...0c4e12a. Read the comment docs.

@rainydio
Copy link
Contributor Author

rainydio commented Aug 20, 2020

I tried to reuse core-api integration test setup for core-magistrate-api by exposing ...plugins argument in core-api test setup function. Apparently not only core-blockchain, but also core-forger and core-api prevent me from doing that. Not gonna pursue that any further.

This in itself is worth merging anyway.

@rainydio rainydio marked this pull request as ready for review August 20, 2020 05:26
@air1one air1one merged commit 3f8727b into develop Aug 21, 2020
@ghost ghost deleted the refactor/core-blockchain/lazy-initialize branch August 21, 2020 13:50
@ghost ghost removed the Status: Needs Review label Aug 21, 2020
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

2 participants