-
Notifications
You must be signed in to change notification settings - Fork 180
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
Introduce the EVM.dryCall
method
#6838
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6838 +/- ##
==========================================
+ Coverage 41.18% 41.19% +0.01%
==========================================
Files 2109 2109
Lines 185660 185716 +56
==========================================
+ Hits 76460 76505 +45
- Misses 102788 102794 +6
- Partials 6412 6417 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
efd6cc6
to
56eeb97
Compare
This new method can be used instead of COA.call, in a Cadence transaction, when the users wants to execute a contract call, without committing any state changes. This is useful for reading a contract's state.
bcfb56f
to
cd3d0ca
Compare
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 work. Left some comments.
fvm/evm/evm_test.go
Outdated
json.MustEncode(cadence.NewUInt64(tx.Gas())), | ||
json.MustEncode(cadence.NewUInt(uint(tx.Value().Uint64()))), | ||
) | ||
_, output, err := vm.Run( |
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.
can you return the execution snapshot out of dryCall
and in "test dryCall has no side-effects"
verify that no changes were made on the EVM state register?
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.
Good point 👍 Added some assertions in 20c84f4
fvm/evm/stdlib/contract.go
Outdated
@@ -178,6 +178,37 @@ var InternalEVMTypeDryRunFunctionType = &sema.FunctionType{ | |||
ReturnTypeAnnotation: sema.NewTypeAnnotation(sema.AnyStructType), | |||
} | |||
|
|||
// InternalEVM.dryCall |
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.
nit: can you move it after call?
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.
Updated in d1e1e8e
fvm/evm/impl/impl.go
Outdated
inter := invocation.Interpreter | ||
locationRange := invocation.LocationRange | ||
|
||
// Get from address | ||
|
||
fromAddressValue, ok := invocation.Arguments[0].(*interpreter.ArrayValue) | ||
if !ok { | ||
panic(errors.NewUnreachableError()) | ||
} | ||
|
||
fromAddress, err := AddressBytesArrayValueToEVMAddress(inter, locationRange, fromAddressValue) | ||
if err != nil { | ||
panic(err) | ||
} | ||
|
||
// Get to address | ||
|
||
toAddressValue, ok := invocation.Arguments[1].(*interpreter.ArrayValue) | ||
if !ok { | ||
panic(errors.NewUnreachableError()) | ||
} | ||
|
||
toAddress, err := AddressBytesArrayValueToEVMAddress(inter, locationRange, toAddressValue) | ||
if err != nil { | ||
panic(err) | ||
} | ||
|
||
to := toAddress.ToCommon() | ||
|
||
// Get data | ||
|
||
dataValue, ok := invocation.Arguments[2].(*interpreter.ArrayValue) | ||
if !ok { | ||
panic(errors.NewUnreachableError()) | ||
} | ||
|
||
data, err := interpreter.ByteArrayValueToByteSlice(inter, dataValue, locationRange) | ||
if err != nil { | ||
panic(err) | ||
} | ||
|
||
// Get gas limit | ||
|
||
gasLimitValue, ok := invocation.Arguments[3].(interpreter.UInt64Value) | ||
if !ok { | ||
panic(errors.NewUnreachableError()) | ||
} | ||
|
||
gasLimit := types.GasLimit(gasLimitValue) | ||
|
||
// Get balance | ||
|
||
balanceValue, ok := invocation.Arguments[4].(interpreter.UIntValue) | ||
if !ok { | ||
panic(errors.NewUnreachableError()) | ||
} |
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.
This first part is exactly the same as a call function with makes sense. Can you create a common function for that?
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.
Good idea 👍 Added in 02715e9
fvm/evm/impl/impl.go
Outdated
|
||
// call contract function | ||
|
||
res := handler.DryRun(txPayload, fromAddress) |
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.
DryRun? should this be called dry call? Is this equivalent?
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.
DryCall
under the hood works the same way as DryRun
. The only difference is the the API. For example:
// EVM.dryRun
EVM.dryRun(tx: tx, from: EVM.EVMAddress(...)) // where tx is an RLP-encoded tx
// EVM.dryCall
EVM.dryCall(
from: EVM.EVMAddress(bytes: [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 10, 0, 0, 0, 0, 0, 0, 0, 0, 15]),
to: EVM.addressFromString(to),
data: data,
gasLimit: gasLimit,
value: EVM.Balance(attoflow: value)
)
Right now EVM.dryRun
is basically only used from the EVM Gateway, for facilitating Cadence Arch Calls in eth_call
/ eth_estimateGas
etc.
The new EVM.dryCall
is supposed to be used from within Cadence transactions, and replace the coa.call
, so that we do not form EVM transactions, when the developer simply wants to read a contract's state.
That's why I am making use of the already existing handler.DryRun
method, which provides the functionality we need. There's no need to add another function.
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! Looks great.
gasLimit: UInt64, | ||
value: Balance, | ||
): Result { | ||
return InternalEVM.dryCall( |
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.
Who is the msg.sender
and txn.origin
in this instance? I would expect dryCall
to be exposed on the COA - coa.call
and dryCall
- but maybe that's just a matter of verbiage.
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.
Who is the msg.sender and txn.origin in this instance?
The msg.sender
for the top-level call, is the from
address. The tx.origin
is also the from
address.
I would expect dryCall to be exposed on the COA - coa.call and dryCall - but maybe that's just a matter of verbiage.
There are a couple of issues with that approach. Most of the APIs for the COA resource, use as from
the COA address. This means that users will have to first create a COA resource, in order to be able to call dryCall
. Even if the said COA resource is destroyed after, the COA contract is still deployed in Flow EVM, so we're left with a dead account.
The second issue, is that users do not have any control over the randomly-generated address of a COA, so they are not able to simulate/debug with an EOA of their choice.
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 think exposing it additionally on the COA makes sense for convenience
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 think exposing it additionally on the COA makes sense for convenience
Ah right, to have it additionally there makes sense indeed. As long as it doesn't force users to create a COA in the first place.
Closes #6800
This new method can be used instead of
COA.call
, in a Cadence transaction, when the user wants to execute a contract call, without committing any state changes. This is useful for reading a contract's state.