Add support for chain.id, block.timestamp, other builtin attributes #208
Conversation
|
The refactoring looks good.
For now I'm thinking keccak256 and the ABI encoding functions will be builtin to certain types as attribute functions, similar to Also, we probably want to limit the use of keccak256 to |
|
Here's the Yul reference btw. |
sbillig
changed the title
Codecov Report
@@ Coverage Diff @@
## master #208 +/- ##
==========================================
+ Coverage 94.28% 94.33% +0.05%
==========================================
Files 49 49
Lines 3357 3356 -1
==========================================
+ Hits 3165 3166 +1
+ Misses 192 190 -2
Continue to review full report at Codecov.
|
|
Ok, this is probably enough for a first pass. msg.data and msg.sig aren't simple yul function calls, so I'm going to wait on those and maybe start looking at something more pressing. The 'use refs in evm_contracts tests' changes are mostly superfluous. I hit a minor ownership issue and thought the nicest solution was for |
| Ok(Object::Block) => todo!(), | ||
| Ok(Object::Chain) => todo!(), | ||
| Ok(Object::Msg) => todo!(), // TODO: error because msg has no methods? | ||
| Ok(Object::Tx) => todo!(), |
There was a problem hiding this comment.
There was a problem hiding this comment.
One of my favorite recent additions!
semantics/src/builtins.rs
Outdated
| pub const MSG: &str = "msg"; | ||
| pub const CLONE: &str = "clone"; | ||
| pub const TO_MEM: &str = "to_mem"; | ||
| use strum::EnumString; |
There was a problem hiding this comment.
Oh, that's cool! I didn't know about EnumString and yeah it makes total sense to use enums here for added guidance from the compiler 🎉
sbillig
force-pushed
the
builtins
branch
2 times, most recently
from
26c6f2e
to
669366b
Compare
|
Rebased on master; should be good now. If I add |
| @@ -124,7 +123,7 @@ impl ContractHarness { | |||
| }) | |||
| .collect::<Vec<_>>(); | |||
|
|
|||
| if !outputs_for_event.contains(&expected_output) { | |||
| if !outputs_for_event.iter().any(|v| &v == expected_output) { | |||
There was a problem hiding this comment.
No big deal but we generally try to avoid single letter variables. This could be named val instead.
| @@ -224,12 +227,12 @@ fn bytes_token(s: &str) -> ethabi::Token { | |||
| ethabi::Token::FixedBytes(ethabi::FixedBytes::from(s)) | |||
| } | |||
|
|
|||
| fn u256_array_token(v: Vec<usize>) -> ethabi::Token { | |||
| ethabi::Token::FixedArray(v.into_iter().map(|n| uint_token(n)).collect()) | |||
| fn u256_array_token(v: &[usize]) -> ethabi::Token { | |||
There was a problem hiding this comment.
But we still have some leftover single letter variable names today...we should adjust all of them (but doesn't have to be in this PR)
|
very nice! |
What was wrong?
There are some missing builtins needed for the uniswap demo.
How was it fixed?
It hasn't been yet :)I'll probably do block.timestamp, maybe more depending on whether they raise tricky issues.
Edit: I've added support for:
block.coinbase
block.difficulty
block.number
block.timestamp
chain.id
msg.value
tx.origin
tx.gas_price (aside: do we want to standardize on snake_case like .to_mem()?)
resolves #190
I changed the builtins to use enums, so the compiler can tell us when we aren't handling a case. I also stubbed out some probable builtins in those enums (based on the existing demo fe code, and vyper/solidity). I'm sure there are some things yet to be decided (eg is keccak256 a method on every value, or a standalone builtin function, or imported from the std library, or part of a builtin trait, or...?). I'm happy to change whatever as decisions are made, of course.
To-Do
OPTIONAL: Update Spec if applicable
Add entry to the release notes (may forgo for trivial changes)
Clean up commit history