hevm: UnitTest.hs: fix nonce initialisation bug (fixes #222) #224
Conversation
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 was an old, unnoticed bug in the unit-test runner, which was brought to light when correct EIP684 collision semantics were recently introduced. The nonce was being incorrectly reset to that of the ethrun account, to adjust for it being potentially incremented during the execution of the test contract's constructor, if that constructor itself contains contract creations. However, since the creation happens on behalf of the test contract, and not the ethrun address, the nonce should be left as is, since replaceCode will propagate the correct nonce. Presumably this mistake was made because prior to commit 070cc9e, the nonce didn't get propagated after a creation at all, so this line was introduced in a flawed attempt to propagate it. Any dapp tests which had creations inside the constructor (including implicit creations that initialise storage variables) would have had their nonces set incorrectly, and were also assigned the wrong addresses. This had gone unnoticed since previously hevm would overwrite on contract collisions, instead of throwing (which is the correct behaviour). Creations in setUp() were not affected.
asymmetric
added a commit
to makerdao/makerpkgs
that referenced
this pull request
Jun 18, 2019
The nonce issue in hevm has been fixed in [224](dapphub/dapptools#224).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
this was an old, unnoticed bug in the unit-test runner, which was
brought to light when correct EIP684 collision semantics were recently
introduced. The nonce was being incorrectly reset to that of the
ethrun account, to adjust for it being potentially incremented during
the execution of the test contract's constructor, if that constructor
itself contains contract creations. However, since the creation
happens on behalf of the test contract, and not the ethrun address,
the nonce should be left as is, since replaceCode will propagate the
correct nonce.
Presumably this mistake was made because prior to commit
070cc9e, the nonce didn't get
propagated after a creation at all, so this line was introduced in a
flawed attempt to propagate it.
Any dapp tests which had creations inside the constructor (including
implicit creations that initialise storage variables) would have had
their nonces set incorrectly, and were also assigned the wrong
addresses. This had gone unnoticed since previously hevm would
overwrite on contract collisions, instead of throwing (which is the
correct behaviour). Creations in setUp() were not affected.
Fixes #222
cc @gbalabasquer @rainbreak