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.

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

hevm: enchanced control over blockchain context #716

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Conversation

d-xo
Copy link
Contributor

@d-xo d-xo commented Jul 29, 2021

  • Adds a DAPP_TEST_NONCE env var that allows users to control the nonce assigned to the test contract
  • Adds unit tests for the various DAPP_TEST_* env vars

cc: @kmbarry1 you were asking for this a while back I believe?

@d-xo d-xo requested a review from MrChico July 29, 2021 10:36
nix/build-dapp-package.nix Outdated Show resolved Hide resolved
@@ -47,6 +48,7 @@ in
'')
passthru.libPaths;
buildPhase = ''
ln -s ${pkgs.writeText "dapprc" dapprc} ./.dapprc
Copy link
Contributor

@asymmetric asymmetric Jul 29, 2021

Choose a reason for hiding this comment

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

Instead of this, it could be nice to have the option to pass a dapprc explicitly as an argument (to dapp build I guess?).

Another (unrelated) nit is that .dapprc should probably be (first) looked up in XDG_CONFIG_HOME (which is not cross-compatible with macOS so would introduce some complexity)

@kmbarry1
Copy link
Contributor

cc: @kmbarry1 you were asking for this a while back I believe?

Hey! Yeah, this gets partially at what I was thinking of. The even more powerful thing would actually be what you suggested in chat, a general hevm cheat code for setting any env value, i.e. hevm.set("nonce", N) or hevm.set("currentAddress", 0x1234), etc.

@d-xo
Copy link
Contributor Author

d-xo commented Jul 29, 2021

Yeah definitely agree re: the cheat code, but that's a bit of a bigger job for another day.

@kmbarry1
Copy link
Contributor

Yeah definitely agree re: the cheat code, but that's a bit of a bigger job for another day.

Sounds good--and I might still take a crack at it if ever I have some free time 😅 .

@d-xo d-xo changed the title hevm: add env var to control nonce hevm: enchanced control over blockchain context Aug 2, 2021
@d-xo
Copy link
Contributor Author

d-xo commented Aug 2, 2021

So I ended up adding the cheatcodes.

I played around with a cheatcode that could change the address of any contract (hevm.file("address", usr, newAddress)), but there are quite a few places in hevm where we expect addresses to stay constant (e.g. we make multiple calls into the same address when running unit tests), and it just felt like it was gonna be more trouble than it was worth to add.

In the end I added the following:

/*
    allows setting the nonce and balance of an arbitrary account:

      hevm.file("nonce", usr, amt)
      hevm.file("balance", usr, amt)
*/
hevm.file(bytes32, address, uint)

/*
    allows setting the caller, origin, and coinbase:

      hevm.file("caller", usr)
      hevm.file("origin", usr)
      hevm.file("coinbase", usr)
*/
hevm.file(bytes32, address)

/*
    allows setting gas price, tx gas limit, block gas limit, and difficulty:

      hevm.file("gasPrice", amt)
      hevm.file("txGasLimit", amt)
      hevm.file("blockGasLimit", amt)
      hevm.file("difficulty", amt)
*/
hevm.file(bytes32, uint)

/*
    allows reading the nonce for any address:

      hevm.look("nonce", usr);
*/
hevm.look(bytes32, address)

/*
    allows reading the txGasLimit

      hevm.look("txGasLimit");
*/
hevm.look(bytes32)

/*
    allows replacing the code at any address (except the currently executing contract)
*/
hevm.replace(address, bytes)

I think these give almost exactly the same level of control as the DAPP_TEST* env vars, but inside of unit tests via cheat codes. I wasn't completely sure about the interface for the look cheat codes, since they both only have one allowed value for the bytes32 param, but I kinda liked the consistency of the interface, open to suggestions if someone has a better idea.

@kmbarry1 I think with replace and file("nonce", val) you should be able to do what you wanted with bytecode verification? I think you should be able to use replace to set the code of the adress you want to deploy from to a contract that just deploys the contract you want to verify and use file to set the nonce. Is that enough or did you need something more?

@@ -2,6 +2,10 @@

## Unreleased

### Added
Copy link
Contributor Author

@d-xo d-xo Aug 2, 2021

Choose a reason for hiding this comment

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

TODO: document new cheatcodes....

@transmissions11
Copy link
Contributor

hollllyyyy cow this is amazing @d-xo. only thing that i think is left out that i would kill to have is the ability to change the address of the test contract like you can with DAPP_TEST_ADDRESS but I know you said thats infeasible 😭

assertEq(who.balance, bal);
}

function testReplace() public {
Copy link
Contributor Author

@d-xo d-xo Aug 2, 2021

Choose a reason for hiding this comment

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

no idea why at all, but if I construct the target contract directly with the code below in the test instead of in the constructor this test always fails (and it does so before reaching the call to the cheatcode).... 🤔

Suggested change
function testReplace() public {
function testReplace() public {
Old target = new Old();

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, strange indeed. Does it revert or what is the error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Failure: testReplace
  src/env.sol:CheatCodes
   ├╴constructor
   │  └╴create <unknown contract>@0xCe71065D4017F316EC606Fe4422e11eB2c47c246 <no source map>
   │     └╴← 63 bytes of code
   └╴testReplace()
      └╴error Revert 0x <no source map>

@d-xo
Copy link
Contributor Author

d-xo commented Aug 3, 2021

only thing that i think is left out that i would kill to have is the ability to change the address of the test contract like you can with DAPP_TEST_ADDRESS

@TransmissionsDev Do you have an example of something that you could do with mutable adresses that you cannot do with mutable code?

src/hevm/src/EVM.hs Outdated Show resolved Hide resolved
@transmissions11
Copy link
Contributor

transmissions11 commented Aug 3, 2021

only thing that i think is left out that i would kill to have is the ability to change the address of the test contract like you can with DAPP_TEST_ADDRESS

@TransmissionsDev Do you have an example of something that you could do with mutable adresses that you cannot do with mutable code?

no, just more convenient hehe. if i was forking mainnet and wanted to adopt the address of some authorized user, much easier to just go hevm.become(0xaa); thing.auth(); vs having to deploy a new contract, that has a method to call auth, etc

will still be awesome tho, forgot about the type(TestContract).creationCode syntax, was imagining it would be harder to load in bytecode

assign (env . contracts . (ix usr) . codeOps) $ mkCodeOps newCode
assign (env . contracts . (ix usr) . opIxMap) $ mkOpIxMap newCode
assign (env . contracts . (ix usr) . contractcode) $ RuntimeCode newCode
assign (cache . fetched . (ix usr) . contractcode) $ RuntimeCode newCode
Copy link
Member

Choose a reason for hiding this comment

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

not sure this is safe. The cache should only contain rpc queries

_ -> vmError (BadCheatCode sig)
_ -> vmError (BadCheatCode sig),

action "replace(address,bytes)" $
Copy link
Member

@MrChico MrChico Sep 4, 2021

Choose a reason for hiding this comment

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

an alternative to this could be move(src,dst) which also moves storage (and nonce and balance?) from the src to dst address

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants