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

Berlin changes #634

Closed
wants to merge 7 commits into from
Closed

Berlin changes #634

wants to merge 7 commits into from

Conversation

laudiacay
Copy link
Contributor

Draft PR for #619 ... Still in a bit of a messy state, but the bulk of the code's written, it just needs to be debugged & passing tests.

Berlin tests that I am starting to work against as of right now are here: https://github.com/ethereum/tests/releases/tag/8.0.3

@laudiacay
Copy link
Contributor Author

I also ran linters against the codebase in 7349aa6 so it would stop crashing my editor- once this is passing tests, if y'all hate the linter output, I will just refactor to take that out.

@d-xo
Copy link
Contributor

d-xo commented Apr 20, 2021

@laudiacay super happy to see this PR, but the linter changes make this very hard to review, could you drop them from the PR please 🙏

src/hevm/src/EVM.hs Outdated Show resolved Hide resolved
src/hevm/src/EVM.hs Outdated Show resolved Hide resolved
src/hevm/src/EVM/ABI.hs Outdated Show resolved Hide resolved
@MrChico
Copy link
Member

MrChico commented Apr 20, 2021

Agree that dropping the linter changes make this a lot nicer. Some of the changes suggested by the linter are nice, like dropping redundant parens and simplifying expressions, but the I don't think we should adhere to its opinion on code alignment

@d-xo
Copy link
Contributor

d-xo commented Apr 20, 2021

Would be happy to review the linter stuff in a seperate pr fwiw

Comment on lines +76 to +80
rlpAccessList = BS.concat $ map (\accessEntry ->
rlpList [BS $ word160Bytes (accessAddress accessEntry),
BS $ rlpList $ map rlpWord256 $ accessStorageKeys accessEntry
]) accessList
Copy link
Member

@MrChico MrChico Apr 20, 2021

Choose a reason for hiding this comment

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

Suggested change
rlpAccessList = BS.concat $ map (\accessEntry ->
rlpList [BS $ word160Bytes (accessAddress accessEntry),
BS $ rlpList $ map rlpWord256 $ accessStorageKeys accessEntry
]) accessList
rlpAccessList = rlpList $ map (\accessEntry ->
BS $ rlpList [BS $ word160Bytes (accessAddress accessEntry)
, BS $ rlpList $ map rlpWord256 $ accessStorageKeys accessEntry]
) accessList

Copy link
Member

Choose a reason for hiding this comment

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

needs confirmation, but I think something like this would make more sense to get the rlp encoding right

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is still wrong, I'm still getting a sender hash that doesn't exist in the prestate of the test. I'll mess with it and see what happens.

@laudiacay
Copy link
Contributor Author

@laudiacay super happy to see this PR, but the linter changes make this very hard to review, could you drop them from the PR please 🙏

done, yeah, i'll PR them elsewhere later with something that removes warnings

@MrChico MrChico mentioned this pull request Apr 20, 2021
@MrChico MrChico closed this Apr 22, 2021
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.

3 participants