This repository has been archived by the owner on Sep 26, 2019. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 130
[PAN-2903] Implement EIP-1344 ChainID Opcode #1690
Merged
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
b931d03
[PAN-2903] Implement ChainID Opcode
shemnon 9941e0a
spotless
shemnon 2632b3d
Merge branch 'master' into EIP1344
shemnon 23e9130
review changes
shemnon 6dfb17d
Merge branch 'master' of github.com:PegaSysEng/pantheon into EIP1344
shemnon File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
39 changes: 39 additions & 0 deletions
39
...eum/core/src/main/java/tech/pegasys/pantheon/ethereum/vm/operations/ChainIdOperation.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
/* | ||
* Copyright 2018 ConsenSys AG. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with | ||
* the License. You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on | ||
* an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the | ||
* specific language governing permissions and limitations under the License. | ||
*/ | ||
package tech.pegasys.pantheon.ethereum.vm.operations; | ||
|
||
import tech.pegasys.pantheon.ethereum.core.Gas; | ||
import tech.pegasys.pantheon.ethereum.vm.AbstractOperation; | ||
import tech.pegasys.pantheon.ethereum.vm.GasCalculator; | ||
import tech.pegasys.pantheon.ethereum.vm.MessageFrame; | ||
import tech.pegasys.pantheon.util.bytes.Bytes32; | ||
|
||
public class ChainIdOperation extends AbstractOperation { | ||
|
||
private final Bytes32 chainId; | ||
|
||
public ChainIdOperation(final GasCalculator gasCalculator, final Bytes32 chainId) { | ||
super(0x46, "CHAINID", 0, 1, false, 1, gasCalculator); | ||
this.chainId = chainId; | ||
} | ||
|
||
@Override | ||
public Gas cost(final MessageFrame frame) { | ||
return gasCalculator().getBaseTierGasCost(); | ||
} | ||
|
||
@Override | ||
public void execute(final MessageFrame frame) { | ||
frame.pushStackItem(chainId); | ||
} | ||
} |
67 changes: 67 additions & 0 deletions
67
...core/src/test/java/tech/pegasys/pantheon/ethereum/vm/operations/ChainIdOperationTest.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,67 @@ | ||
/* | ||
* Copyright 2018 ConsenSys AG. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with | ||
* the License. You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on | ||
* an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the | ||
* specific language governing permissions and limitations under the License. | ||
*/ | ||
package tech.pegasys.pantheon.ethereum.vm.operations; | ||
|
||
import static org.assertj.core.api.Assertions.assertThat; | ||
import static org.mockito.Mockito.mock; | ||
|
||
import tech.pegasys.pantheon.ethereum.core.Gas; | ||
import tech.pegasys.pantheon.ethereum.mainnet.ConstantinopleGasCalculator; | ||
import tech.pegasys.pantheon.ethereum.vm.MessageFrame; | ||
import tech.pegasys.pantheon.util.bytes.Bytes32; | ||
|
||
import org.junit.Test; | ||
import org.junit.runner.RunWith; | ||
import org.junit.runners.Parameterized; | ||
import org.junit.runners.Parameterized.Parameters; | ||
import org.mockito.ArgumentCaptor; | ||
import org.mockito.Mockito; | ||
|
||
@RunWith(Parameterized.class) | ||
public class ChainIdOperationTest { | ||
|
||
private final Bytes32 chainId; | ||
private final int expectedGas; | ||
private final MessageFrame messageFrame = mock(MessageFrame.class); | ||
private final ChainIdOperation operation; | ||
|
||
@Parameters(name = "chainId: {0}, expectedGas: {1}") | ||
public static Object[][] params() { | ||
return new Object[][] { | ||
{"0x01", 2}, | ||
{"0x03", 2}, | ||
{"0x04", 2}, | ||
{"0x05", 2}, | ||
}; | ||
} | ||
|
||
public ChainIdOperationTest(final String chainIdString, final int expectedGas) { | ||
chainId = Bytes32.fromHexString(chainIdString); | ||
this.expectedGas = expectedGas; | ||
operation = new ChainIdOperation(new ConstantinopleGasCalculator(), chainId); | ||
} | ||
|
||
@Test | ||
public void shouldReturnChainId() { | ||
final ArgumentCaptor<Bytes32> arg = ArgumentCaptor.forClass(Bytes32.class); | ||
operation.execute(messageFrame); | ||
Mockito.verify(messageFrame).pushStackItem(arg.capture()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You should probably also verify that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
Mockito.verifyNoMoreInteractions(messageFrame); | ||
assertThat(arg.getValue()).isEqualByComparingTo(chainId); | ||
} | ||
|
||
@Test | ||
public void shouldCalculateGasPrice() { | ||
assertThat(operation.cost(messageFrame)).isEqualTo(Gas.of(expectedGas)); | ||
} | ||
} |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
If you trace back to where the
chainId
is set, it looks like it's only ever missing in test cases. Might be worth going through and making thechainId
argument a plain unwrappedBigInteger
, in which case we wouldn't need this check.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.
FixedDifficultyProtocolSchedule uses it too. It's at least a 4 file change, larger than this PR, so that level of change would deserve its own PR.
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.
There's no requirement that a chain has a
chainId
- it may be omitted from the genesis config. This is a slightly interesting hole in the ChainID opcode spec. Doesn't apply to any public chain but there may be chains out there it does affect. So maybe ChainID opcode should return 0 when there is no chain ID?