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

Remove RskSystemProperties from many core objects #642

Merged
merged 9 commits into from Sep 21, 2018

Conversation

Projects
None yet
6 participants
@diega
Contributor

diega commented Sep 9, 2018

No description provided.

@diega diega requested review from tinchou, adauto82 and lsebrie Sep 9, 2018

@diega diega force-pushed the remove_config_blockchain branch 3 times, most recently from 7511038 to 2cc5c9e Sep 9, 2018

@diega diega changed the title from Cleanup Blockchain implementation to Remove RskSystemProperties from many core objects Sep 9, 2018

@diega diega force-pushed the remove_config_blockchain branch 2 times, most recently from 82a3129 to afe2aa9 Sep 9, 2018

@tinchou

This comment has been minimized.

Contributor

tinchou commented Sep 10, 2018

Looking good! I would discuss the flushing and Remove config object from BlockExecutor changes before merging.

@diega diega force-pushed the remove_config_blockchain branch 5 times, most recently from 863508d to a43aa04 Sep 10, 2018

@diega

This comment has been minimized.

Contributor

diega commented Sep 11, 2018

@tinchou I removed the commit regarding the flush

tx, 0, coinbase, repository, blockStore, receiptStore,
programInvokeFactory, executionBlock, new EthereumListenerAdapter(), 0, config.getVmConfig(),
config.getBlockchainConfig(), config.playVM(), config.isRemascEnabled(), config.vmTrace(), new PrecompiledContracts(config),
config.databaseDir(), config.vmTraceDir(), config.vmTraceCompressed()

This comment has been minimized.

@lsebrie

lsebrie Sep 11, 2018

Contributor

Here you could rework config.getVmConfig() object to have all vm config data related and reduce parameters

This comment has been minimized.

@tinchou

tinchou Sep 11, 2018

Contributor

why do we even have a remascEnabled option? 😭

This comment has been minimized.

@adauto82

adauto82 Sep 12, 2018

Contributor

Maybee it's time
//TODO: REMOVE THIS WHEN THE LocalBLockTests starts working with REMASC
public boolean isRemascEnabled() { return remascEnabled; }

@adauto82

We should get together and discuss what is the future of configuration for our node.

@diega diega dismissed stale reviews from adauto82 and tinchou via 23f2752 Sep 11, 2018

@diega diega force-pushed the remove_config_blockchain branch 2 times, most recently from 23f2752 to 5f20d73 Sep 11, 2018

@aeidelman

This comment has been minimized.

Contributor

aeidelman commented Sep 18, 2018

Please update branch

diega added some commits Sep 9, 2018

diega added some commits Sep 9, 2018

@diega diega force-pushed the remove_config_blockchain branch from 5f20d73 to e47e22d Sep 19, 2018

@rskops

This comment has been minimized.

rskops commented Sep 19, 2018

SonarQube analysis reported 8 issues

  1. MAJOR LevelDbDataSource.java#L97: java/nio/file/Paths.get(Ljava/lang/String;[Ljava/lang/String;)Ljava/nio/file/Path; reads a file whose location might be specified by user input rule
  2. MAJOR LevelDbDataSource.java#L100: java/nio/file/Paths.get(Ljava/lang/String;[Ljava/lang/String;)Ljava/nio/file/Path; reads a file whose location might be specified by user input rule
  3. MAJOR VMUtils.java#L56: java/nio/file/Paths.get(Ljava/lang/String;[Ljava/lang/String;)Ljava/nio/file/Path; reads a file whose location might be specified by user input rule
  4. MINOR BlockChainImpl.java#L100: Constructor has 9 parameters, which is greater than 7 authorized. rule
  5. MINOR TransactionPoolImpl.java#L160: Reduce this lambda expression number of lines from 22 to at most 20. rule
  6. MINOR BlockToMineBuilder.java#L97: Reduce this lambda expression number of lines from 21 to at most 20. rule
  7. MINOR TransactionExecutor.java#L103: Constructor has 19 parameters, which is greater than 7 authorized. rule
  8. MINOR BlockChainLoader.java#L95: Reduce this lambda expression number of lines from 21 to at most 20. rule
@tinchou

Re-approve after merge

@diega diega merged commit c909d88 into master Sep 21, 2018

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
default Build finished.
Details
sonarqube SonarQube reported 8 issues, no criticals or blockers

@diega diega deleted the remove_config_blockchain branch Sep 21, 2018

@aeidelman aeidelman added this to the Orchid v0.5.1 milestone Oct 2, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment