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

Runtime testing #243

Merged
merged 1 commit into from
Feb 12, 2021
Merged

Runtime testing #243

merged 1 commit into from
Feb 12, 2021

Conversation

g-r-a-n-t
Copy link
Member

@g-r-a-n-t g-r-a-n-t commented Feb 10, 2021

What was wrong?

closes #214

How was it fixed?

The solution I used was a bit different than the one proposed in the linked issue. Instead of testing functions in the runtime object of a given contract, the functions are brought in directly from the runtime builder and tested. This seemed like a much more straight forward way of doing things.

  • reorganized the test directory
  • add a new testing utility function test_runtime_functions that takes a vector of functions to be tested and a vector of statements to test those functions
  • added a handy assert_eq macro that builds an if statement that enters and reverts if the assertion is false
  • used the utilities described above to test some standard runtime functions

To-Do

  • Get to the bottom of the apparent sloadn bug

  • Add entry to the release notes (may forgo for trivial changes)

  • Clean up commit history

compiler/src/yul/runtime/mod.rs Outdated Show resolved Hide resolved
compiler/src/evm/mod.rs Outdated Show resolved Hide resolved
@g-r-a-n-t g-r-a-n-t force-pushed the runtime-testing branch 3 times, most recently from 5866d49 to b6c45fd Compare February 12, 2021 00:26
@codecov-io
Copy link

Codecov Report

Merging #243 (b6c45fd) into master (c39d75b) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #243   +/-   ##
=======================================
  Coverage   93.91%   93.91%           
=======================================
  Files          54       54           
  Lines        3744     3744           
=======================================
  Hits         3516     3516           
  Misses        228      228           
Impacted Files Coverage Δ
compiler/src/yul/mod.rs 100.00% <ø> (ø)
compiler/src/yul/runtime/mod.rs 95.00% <ø> (ø)
compiler/src/evm/mod.rs 95.83% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c39d75b...b6c45fd. Read the comment docs.

(sstore(100, a))

// this fails for some reason.. possible bug?
// [assert_eq!(b, (sloadn(130, 2)))]
Copy link
Member Author

Choose a reason for hiding this comment

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

it looks like this might be a bug @cburgdorf. I could be missing something tho

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is interesting. You can prove [assert_eq!(0x42, (sloadn(100, 1)))] but that's as far as I got. I tried changing the second byte to anything but 00 and then use sloadn(101, 1) but that already fails. So, maybe a base assumption about how storage works is wrong? 😅

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'm going to merge this and continue digging into the storage stuff. Pretty cool that we already found an issue with this testing.

@g-r-a-n-t
Copy link
Member Author

still a few todo items to check off, but this should be good for a review

Copy link
Collaborator

@cburgdorf cburgdorf left a comment

Choose a reason for hiding this comment

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

Really cool! I played a bit with the code and tried to find an answer to your inline comment..it taught me a bit but I had no luck getting to the bottom of it.

compiler/tests/evm_runtime.rs Show resolved Hide resolved
compiler/tests/evm_runtime.rs Show resolved Hide resolved
compiler/tests/evm_runtime.rs Outdated Show resolved Hide resolved
(sstore(100, a))

// this fails for some reason.. possible bug?
// [assert_eq!(b, (sloadn(130, 2)))]
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is interesting. You can prove [assert_eq!(0x42, (sloadn(100, 1)))] but that's as far as I got. I tried changing the second byte to anything but 00 and then use sloadn(101, 1) but that already fails. So, maybe a base assumption about how storage works is wrong? 😅

@g-r-a-n-t g-r-a-n-t merged commit 9c49805 into ethereum:master Feb 12, 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.

Contract runtime testing.
3 participants