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.

Sign up for GitHub

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

Jump to bottom

Remove RskSystemProperties from many core objects #642

Merged
merged 9 commits into from
Sep 21, 2018
Merged

Remove RskSystemProperties from many core objects #642

merged 9 commits into from
Sep 21, 2018

Conversation

diega
Copy link
Contributor

@diega diega commented Sep 9, 2018

No description provided.

@diega diega force-pushed the remove_config_blockchain branch 3 times, most recently from 7511038 to 2cc5c9e Compare September 9, 2018 21:47
@diega diega changed the title Cleanup Blockchain implementation 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 Compare September 10, 2018 11:38
@colltoaction
Copy link
Contributor

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 Compare September 11, 2018 13:29
@diega
Copy link
Contributor Author

diega commented Sep 11, 2018

@tinchou I removed the commit regarding the flush

lsebrie
lsebrie reviewed Sep 11, 2018
View reviewed changes
rskj-core/src/main/java/co/rsk/core/ReversibleTransactionExecutor.java
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()
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

why do we even have a remascEnabled option? 😭

Copy link
Contributor

@adauto82 adauto82 Sep 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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

colltoaction
colltoaction previously approved these changes Sep 11, 2018
View reviewed changes
adauto82
adauto82 previously approved these changes Sep 11, 2018
View reviewed changes
Copy link
Contributor

@adauto82 adauto82 left a comment

Choose a reason for hiding this comment

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

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

@diega diega dismissed stale reviews from adauto82 and colltoaction via 23f2752 September 11, 2018 16:32
@diega diega force-pushed the remove_config_blockchain branch 2 times, most recently from 23f2752 to 5f20d73 Compare September 12, 2018 14:59
colltoaction
colltoaction previously approved these changes Sep 13, 2018
View reviewed changes
@aeidelman
Copy link
Collaborator

Please update branch

@diega diega force-pushed the remove_config_blockchain branch from 5f20d73 to e47e22d Compare September 19, 2018 17:22
@rskops
Copy link
Collaborator

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

colltoaction
colltoaction approved these changes Sep 19, 2018
View reviewed changes
Copy link
Contributor

@colltoaction colltoaction left a comment

Choose a reason for hiding this comment

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

Re-approve after merge

@diega diega merged commit c909d88 into master Sep 21, 2018
@diega diega deleted the remove_config_blockchain branch September 21, 2018 17:19
@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
Reviewers

@adauto82 adauto82 adauto82 left review comments

@lsebrie lsebrie lsebrie left review comments

@colltoaction colltoaction colltoaction approved these changes

Assignees
No one assigned
Labels
tech debt & clean code
Projects
None yet
Milestone
Orchid 0.5.1
Development

Successfully merging this pull request may close these issues.
6 participants
@diega @colltoaction @aeidelman @rskops @adauto82 @lsebrie