Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

fix: make forking chainId aware #1537

Merged
merged 7 commits into from
Nov 18, 2021
Merged

Conversation

davidmurdoch
Copy link
Member

@davidmurdoch davidmurdoch commented Nov 11, 2021

fixes #628

This change causes ganache to use its default chainId instead of the fork's chainId. This prevents replay attacks, but breaks debug_traceTransaction and eth_call. So this PR also makes debug_traceTransaction and eth_call "aware" of the chainId of the original chain so that they can use the correct chainId in block contexts that require it (on or before the fork block number).

contractName: "EthCall"
});
});

Copy link
Member Author

Choose a reason for hiding this comment

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

When debugging fork tests this contract complication keep popping up. It should never have been outside a before call anyway.

@@ -1,3 +1,4 @@
// SPDX-License-Identifier: MIT
Copy link
Member Author

Choose a reason for hiding this comment

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

silencing a warning from the solc compiler

@@ -32,7 +32,7 @@ contract Forking {
// initial value is 0, set to 1 in constructor
uint public value4 = 0;

constructor() public payable {
constructor() payable {
Copy link
Member Author

Choose a reason for hiding this comment

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

The solc compiler was warning me about public, so I removed it.


function getChainId() public pure returns (uint256 chainId) {
assembly {
chainId := chainid()
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the test change you should actually care about. :-)

@@ -15,6 +16,42 @@ import compile from "../helpers/compile";
import path from "path";
import { CodedError } from "@ganache/ethereum-utils";

async function deployContract(
Copy link
Member Author

Choose a reason for hiding this comment

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

I just moved this to a more global scope so I could reuse it.

`Invalid chain id (${this.chainId.toNumber()}) for chain with id ${common.chainId()}.`,
JsonRpcErrorCode.INVALID_INPUT
);
}
} else {
Copy link
Member Author

Choose a reason for hiding this comment

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

Deleted because I need this to happen. Maaayyybbbe we could work around it, but I'm not sure it'll be a problem since data.chainId should only be coming from the database? 🤔 hmmmm

davidmurdoch and others added 2 commits November 16, 2021 14:15
Co-authored-by: Micaiah Reid <micaiahreid@gmail.com>
Co-authored-by: Micaiah Reid <micaiahreid@gmail.com>
@davidmurdoch davidmurdoch merged commit 469d198 into develop Nov 18, 2021
@davidmurdoch davidmurdoch deleted the fix/fork-fake-chain-id branch November 18, 2021 00:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

investigate if forking should use the chainId of the original chain
2 participants