-
Notifications
You must be signed in to change notification settings - Fork 48
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
Make Ethereum tests pass #56
Conversation
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.
I'm somewhat worried about the changes in these unit tests. I haven't look at the entire changes yet, but will try to do soon.
pushed a fix for an off by one error in |
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.
Epic PR <3. Looks mostly very good, just a few comments & questions.
It would also be great to add a CI stage that runs these tests to make sure we don't merge regressions here.
ConcreteStore s -> mempty -- clearZeroStorage $ fromMaybe mempty $ Map.lookup (num addr) s | ||
EmptyStore -> mempty | ||
AbstractStore -> mempty -- error "AbstractStore, should this be handled?" | ||
SStore {} -> mempty -- error "SStore, should this be handled?" |
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.
what needs to be done here? I think we can safely error out on any symbolic storage expressions here since VMTests should always be concrete...
{- | ||
clearOrigStorage :: EVM.Contract -> EVM.Contract | ||
clearOrigStorage = undefined | ||
-} |
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 this still needed?
@@ -114,72 +116,77 @@ checkExpectation diff x vm = do | |||
return okState | |||
|
|||
-- quotient account state by nullness | |||
(~=) :: Map Addr EVM.Contract -> Map Addr EVM.Contract -> Bool | |||
(~=) cs cs' = | |||
(~=) :: Map Addr (EVM.Contract, Storage) -> Map Addr (EVM.Contract, Storage) -> Bool |
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.
Should we use the ContractWithStorage
type here?
clearOrigStorage :: EVM.Contract -> EVM.Contract | ||
clearOrigStorage = undefined | ||
-} | ||
|
||
-- TODO: why is this needed? |
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.
good question hehe, any ideas? I'm not super familiar with the VMTest format...
Thanks again @arcz |
Description
Closes #9
Checklist