-
Notifications
You must be signed in to change notification settings - Fork 130
Conversation
724e125
to
ef950b8
Compare
ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/mainnet/ProtocolScheduleFactory.java
Outdated
Show resolved
Hide resolved
import tech.pegasys.pantheon.ethereum.core.PrivacyParameters; | ||
import tech.pegasys.pantheon.ethereum.vm.GasCalculator; | ||
|
||
public class PrecompiledContractConfiguration { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any utility to this class beyond holding 2 objects? Could its consumers take the gas calculator and privacy parameters separately without losing functionality?
ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/mainnet/ProtocolScheduleFactory.java
Outdated
Show resolved
Hide resolved
...c/main/java/tech/pegasys/pantheon/ethereum/mainnet/MainnetPrecompiledContractRegistries.java
Outdated
Show resolved
Hide resolved
ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/mainnet/MainnetProtocolSpecs.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As indicated in the comment we are working on an update of the CLI guideline that will be released in the 0.9 version. This will introduce breaking changes and CLI components naming conventions. So to prevent a refactoring of this part of your code, you could already make the listed change (only one) to be sure it will easily fit in the next release. Thanks for your valuable contribution.
pantheon/src/main/java/tech/pegasys/pantheon/cli/PantheonCommand.java
Outdated
Show resolved
Hide resolved
pantheon/src/main/java/tech/pegasys/pantheon/cli/PantheonCommand.java
Outdated
Show resolved
Hide resolved
ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/mainnet/ProtocolSpecBuilder.java
Show resolved
Hide resolved
642a7c5
to
1125277
Compare
wanted to only approve CLI option related changes, but it seems that it validates all... so reverting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me for the CLI option part.
64d29d2
to
fb706cc
Compare
5abb5bb
to
0627dc1
Compare
ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/core/PrivacyParameters.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Some minor comments.
0627dc1
to
52757fe
Compare
PR description
Add functionality to include privacy pre-compiled contract if privacy is enabled through CLI.
The purpose of the PR is to take the privacy CLI commands and appropriately include
PrivatePrecompiledContract
to protocol spec. Currently the contract is stubbed.