-
Notifications
You must be signed in to change notification settings - Fork 179
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
[Flow EVM] extend EVM precompiles with cadence arch #5233
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #5233 +/- ##
==========================================
- Coverage 55.47% 55.42% -0.06%
==========================================
Files 996 993 -3
Lines 95787 95514 -273
==========================================
- Hits 53136 52935 -201
+ Misses 38661 38602 -59
+ Partials 3990 3977 -13
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
Nice!
fvm/evm/precompiles/precompile.go
Outdated
// RequiredPrice calculates the contract gas use | ||
func (p *precompile) RequiredGas(input []byte) uint64 { | ||
if len(input) < 4 { | ||
return 0 |
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.
Why do shorter inputs not require any gas?
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 added a small gas of 1 to be returned but inputs shorter than 4 mean we don't have a proper methodID extractable so we are going to error out on the run section.
@ramtinms Apologies, I hadn't realized this was still in draft when I reviewed it |
@turbolent you were too fast, I accidentally pressed ready to be reviewed and changed it back but you got it reviewed :D |
// BlockContext holds the context needed for the emulator operations | ||
type BlockContext struct { | ||
BlockNumber uint64 | ||
DirectCallBaseGasUsage uint64 | ||
DirectCallGasPrice uint64 | ||
GasFeeCollector Address | ||
|
||
// a set of extra precompiles to be injected | ||
ExtraPrecompiles []Precompile |
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.
FlowPrecompiles
?
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 trying to keep the emulator not referencing any Flow protocol concepts, we might even add some precompiles in the future that is not Flow-related.
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.
Left some comments, but generally looks good. Nice!
@turbolent now this is ready, do you want to take another look? |
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.
Great work! Just some minor suggestions
fvm/evm/handler/addressAllocator.go
Outdated
) | ||
|
||
var ( | ||
// prefixes: | ||
// the first 12 bytes of addresses allocation | ||
// leading zeros helps with storage and all zero is reserved for the EVM precompiles | ||
FlowEVMPrecompileAddressPrefix = [...]byte{0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1} | ||
FlowEVMCOAAddressPrefix = [...]byte{0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 2} | ||
) |
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.
Given that the prefix is/must be 12 bytes, define a constant and use it here, as well in the related copy calls below:
) | |
var ( | |
// prefixes: | |
// the first 12 bytes of addresses allocation | |
// leading zeros helps with storage and all zero is reserved for the EVM precompiles | |
FlowEVMPrecompileAddressPrefix = [...]byte{0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1} | |
FlowEVMCOAAddressPrefix = [...]byte{0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 2} | |
) | |
addressPrefixLen = 8 | |
) | |
var ( | |
// prefixes: | |
// the first 12 bytes of addresses allocation | |
// leading zeros helps with storage and all zero is reserved for the EVM precompiles | |
FlowEVMPrecompileAddressPrefix = [addressPrefixLen]byte{0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1} | |
FlowEVMCOAAddressPrefix = [addressPrefixLen]byte{0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 2} | |
) |
fvm/evm/handler/addressAllocator.go
Outdated
return makePrefixedAddress(index, FlowEVMPrecompileAddressPrefix) | ||
} | ||
|
||
func makePrefixedAddress(index uint64, prefix [12]byte) types.Address { |
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.
func makePrefixedAddress(index uint64, prefix [12]byte) types.Address { | |
func makePrefixedAddress(index uint64, prefix [addressPrefixLen]byte) types.Address { |
) | ||
|
||
func TestMutiFunctionContract(t *testing.T) { | ||
address := testutils.RandomAddress(t) |
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.
address := testutils.RandomAddress(t) | |
t.Parallel() | |
address := testutils.RandomAddress(t) |
fvm/evm/precompiles/precompile.go
Outdated
|
||
// Run runs the precompiled contract | ||
func (p *precompile) Run(input []byte) ([]byte, error) { | ||
if len(input) < 4 { |
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.
if len(input) < 4 { | |
if len(input) < FunctionSelectorLength { |
fvm/evm/precompiles/precompile.go
Outdated
|
||
// RequiredGas calculates the contract gas use | ||
func (p *precompile) RequiredGas(input []byte) uint64 { | ||
if len(input) < 4 { |
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.
if len(input) < 4 { | |
if len(input) < FunctionSelectorLength { |
) | ||
|
||
func TestFunctionSelector(t *testing.T) { | ||
expected := gethCrypto.Keccak256([]byte("test()"))[:4] |
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.
expected := gethCrypto.Keccak256([]byte("test()"))[:4] | |
t.Parallel() | |
expected := gethCrypto.Keccak256([]byte("test()"))[:FunctionSelectorLength] |
closes: #5199