Add ecrecover() builtin for EVM #1543
Conversation
|
Is there a test for this change? |
seanyoung
force-pushed
the
ecrecover
branch
2 times, most recently
from
ab69f8f to
252848f
Compare
Now there is 😊 |
| @@ -2186,6 +2186,56 @@ fn expr_builtin( | |||
| expr: Box::new(codegen_expr), | |||
| } | |||
| } | |||
| ast::Builtin::ECRecover => { | |||
There was a problem hiding this comment.
Is it possible to use Instr::Unimplemented here?
There was a problem hiding this comment.
Why Instr::Unimplemented preferable over an actual implementation?
There was a problem hiding this comment.
My reasoning is that as you cannot test this portion of code, we could better just use Instr::Unimplemented as we did so for other Ethereum builtins. This would not diminish our test coverage numbers.
There was a problem hiding this comment.
Which test coverage numbers?
There was a problem hiding this comment.
Yes, yolo in code in which we can't test isn't a good practice
There was a problem hiding this comment.
ok, I've been outvoted, it's gone
There was a problem hiding this comment.
I mean it could also be commented out, saying that this is how we think it should be implemented, but it's commented out and an Unreachable instead until we can test it?
There was a problem hiding this comment.
Good idea. I've added the code I wrote as a comment
src/sema/ast.rs
Outdated
| @@ -1658,6 +1658,8 @@ pub enum Builtin { | |||
| Accounts, | |||
| UserTypeWrap, | |||
| UserTypeUnwrap, | |||
| /// EVM only, not implemented yet. This is useful for anyone using Solang to create an AST for EVM contracts | |||
There was a problem hiding this comment.
The contracts pallet provides ecdsa recovery
| @@ -2186,6 +2186,56 @@ fn expr_builtin( | |||
| expr: Box::new(codegen_expr), | |||
| } | |||
| } | |||
| ast::Builtin::ECRecover => { | |||
There was a problem hiding this comment.
Yes, yolo in code in which we can't test isn't a good practice
seanyoung
force-pushed
the
ecrecover
branch
2 times, most recently
from
90ce9d2 to
488126d
Compare
Fixes hyperledger-solang#1536 Signed-off-by: Sean Young <[email protected]>
seanyoung
force-pushed
the
ecrecover
branch
from
488126d to
aa476a4
Compare
Fixes #1536