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

Implement the eth_call JSON-RPC endpoint #24

Merged
merged 5 commits into from
Jan 31, 2024

Conversation

m-Peter
Copy link
Collaborator

@m-Peter m-Peter commented Jan 23, 2024

Closes: #9

Description

Functionality to execute read-only function calls.


For contributor use:

  • Targeted PR against master branch
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the standards mentioned here.
  • Updated relevant documentation
  • Re-reviewed Files changed in the Github PR explorer
  • Added appropriate labels

@m-Peter m-Peter added this to the Flow-EVM-M2 milestone Jan 23, 2024
@m-Peter m-Peter self-assigned this Jan 23, 2024
@m-Peter m-Peter marked this pull request as ready for review January 29, 2024 17:27
@m-Peter m-Peter requested a review from sideninja January 29, 2024 17:27
Copy link
Contributor

@sideninja sideninja left a comment

Choose a reason for hiding this comment

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

You can use the client interface from go-sdk https://github.com/onflow/flow-go-sdk/blob/master/access/client.go#L34
that way you don't have to redefine the interface, and the mocks are also already available from go-sdk if you want to use for testing.

@m-Peter
Copy link
Collaborator Author

m-Peter commented Jan 29, 2024

@sideninja Good call out 👍 I did update the code to use the access.Client interface in 655c8f5. However, the mocks from flow-go-sdk are not usable in our case. For example:
What the mocks have

func (_m *MockRPCClient) ExecuteScriptAtLatestBlock(ctx context.Context, in *access.ExecuteScriptAtLatestBlockRequest, opts ...grpc.CallOption) (*access.ExecuteScriptResponse, error) {

vs what we want

func (_m *MockAccessClient) ExecuteScriptAtLatestBlock(ctx context.Context, script []byte, arguments []cadence.Value) (cadence.Value, error) {

I have generated a mock implementation of access.Client though, with a workaround, since it is defined on a different repository.

@m-Peter m-Peter requested a review from sideninja January 29, 2024 20:34
api/api.go Outdated
Comment on lines 677 to 694
import EVM from 0xf8d6e0586b0a20c7

access(all)
fun main(data: [UInt8], contractAddress: [UInt8; 20]): [UInt8] {
let flowAccount = getAuthAccount<auth(Storage) &Account>(Address(0xf8d6e0586b0a20c7))
let bridgedAccount = flowAccount.storage.borrow<&EVM.BridgedAccount>(
from: /storage/evm
) ?? panic("Could not borrow reference to the bridged account!")

let evmResult = bridgedAccount.call(
to: EVM.EVMAddress(bytes: contractAddress),
data: data,
gasLimit: 300000,
value: EVM.Balance(flow: 0.0)
)

return evmResult
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I would extract this into a call.cdc file and then embed it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, good point 👍
I have updated that in 86a9487 and added some TODOs for interpolating some parts of the script.

@@ -0,0 +1,813 @@
// Code generated by mockery v2.21.4. DO NOT EDIT.
Copy link
Contributor

Choose a reason for hiding this comment

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

was this copied over or generated? if it was copied it's better if generated, just add a comment for generation on top of FlowAccessClient and use mockery.

Copy link
Contributor

Choose a reason for hiding this comment

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

I even think it would be useful to create a PR against flow-go-sdk and generate the mock there for the interface:
https://github.com/onflow/flow-go-sdk/blob/master/access/client.go#L34

Then you could just use that mock and it would also help others.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was generated with mockery, I have added a relevant Makefile recipe as well in: 1b20bb5.
I will also open up a PR to do this in flow-go-sdk, however, it might take some time to get that change in. So for the time being, we can stick to the mock generated in this repository.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok great. As long as it's being generated here it's all good, and the PR in go-sdk would be good to have but not a blocker yeah

Comment on lines 5 to 7
type FlowAccessClient interface {
access.Client
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
type FlowAccessClient interface {
access.Client
}
type FlowAccessClient access.Client

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unfortunately, this does not work for mockery. I think it needs an interface definition to work, and not a type "alias".
I had a look at flow-go(onflow/flow-go@02e4142), and the Proxy interface is how they mock implementations for interfaces defined in 3rd party packages.

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting.

@m-Peter m-Peter requested a review from sideninja January 31, 2024 10:13
@m-Peter m-Peter merged commit cb7c919 into onflow:main Jan 31, 2024
1 check passed
@m-Peter m-Peter deleted the eth-call-endpoint branch January 31, 2024 14:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

Implement API methods which require Cadence scripts to be executed
2 participants