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.

Sign up for GitHub

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

Jump to bottom

Keystone: EVM write capability + forwarder contract #12045

Merged
merged 7 commits into from
Feb 16, 2024
Merged

Keystone: EVM write capability + forwarder contract #12045

merged 7 commits into from
Feb 16, 2024

Conversation

archseer
Copy link
Contributor

No description provided.

@github-actions GitHub Actions
Copy link
Contributor

I see that you haven't updated any CHANGELOG files. Would it make sense to do so?

archseer
archseer commented Feb 15, 2024
View reviewed changes
core/gethwrappers/keystone/go_generate.go

// Keystone

//go:generate go run ../generation/generate/wrap.go ../../../contracts/solc/v0.8.19/KeystoneForwarder/KeystoneForwarder.abi ../../../contracts/solc/v0.8.19/KeystoneForwarder/KeystoneForwarder.bin KeystoneForwarder forwarder
Copy link
Contributor Author

Choose a reason for hiding this comment

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

NOTE: getting the solc compiler wrapper working was really tedious so I ended up using forge build --extra-output-files abi then abigen --abi ../../../contracts/foundry-artifacts/KeystoneForwarder.sol/KeystoneForwarder.abi.json --pkg forwarder --out generated/forwarder/forwarder.go

@github-actions GitHub Actions
Copy link
Contributor

Go solidity wrappers are out-of-date, regenerate them via the make wrappers-all command

archseer
archseer commented Feb 15, 2024
View reviewed changes
core/capabilities/targets/write_target_test.go
Comment on lines +49 to +60
config, err := values.NewMap(map[string]any{
"abi": "receive(report bytes)",
"params": []any{"$(report)"},
})
require.NoError(t, err)

inputs, err := values.NewMap(map[string]any{
"report": []byte{1, 2, 3},
})
require.NoError(t, err)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

See here for example inputs and config

bolekk
bolekk reviewed Feb 16, 2024
View reviewed changes
Copy link
Contributor

@bolekk bolekk left a comment

Choose a reason for hiding this comment

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

Some initial comments.

common/txmgr/types/tx.go Outdated Show resolved Hide resolved
contracts/src/v0.8/keystone/KeystoneForwarder.sol Outdated Show resolved Hide resolved
contracts/src/v0.8/keystone/KeystoneForwarder.sol Outdated
}

// abi.encodeWithSelector(bytes4 selector, data...)
(bool success, bytes memory result) = targetAddress.call(data);
Copy link
Contributor

Choose a reason for hiding this comment

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

This can revert, should we call it in try-catch so that s_reports gets updated regardless?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

address.call actually doesn't revert, that's why there's the bool success parameter! Had to double check, compiler won't actually accept try targetAddress.call(data) returns (bool success, bytes memory result)

core/capabilities/targets/write_target.go Outdated Show resolved Hide resolved
core/capabilities/targets/write_target_test.go
err := method.Inputs.UnpackIntoMap(payload, req.EncodedPayload[4:])
require.NoError(t, err)
require.Equal(t, []byte{
0xa6, 0x9b, 0x6e, 0xd0, // selector = keccak(signature)[:4]
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer a hex-encoded string for readability.

core/chains/evm/abi/selector_parser.go Outdated Show resolved Hide resolved
core/capabilities/targets/write_target.go

txMeta := &txmgr.TxMeta{
// FwdrDestAddress: ,
WorkflowExecutionID: &request.Metadata.WorkflowExecutionID,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this? Is TXM going to do anything special with the ID?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the tx has a SignalCallback field that's currently used by the broadcaster to call into the task pipeline to resume a pipeline when a tx is confirmed. I'm planning to adjust that logic so that if there's a execution ID set, it'll instead call into the capability which will then send a result down the channel

@github-actions GitHub Actions
Copy link
Contributor

Go solidity wrappers are out-of-date, regenerate them via the make wrappers-all command

@github-actions GitHub Actions
Copy link
Contributor

Go solidity wrappers are out-of-date, regenerate them via the make wrappers-all command

@github-actions GitHub Actions
Copy link
Contributor

Go solidity wrappers are out-of-date, regenerate them via the make wrappers-all command

1 similar comment
@github-actions GitHub Actions
Copy link
Contributor

Go solidity wrappers are out-of-date, regenerate them via the make wrappers-all command

@github-actions GitHub Actions
Copy link
Contributor

Go solidity wrappers are out-of-date, regenerate them via the make wrappers-all command

@github-actions GitHub Actions
Copy link
Contributor

Go solidity wrappers are out-of-date, regenerate them via the make wrappers-all command

@github-actions GitHub Actions
Copy link
Contributor

Go solidity wrappers are out-of-date, regenerate them via the make wrappers-all command

@github-actions GitHub Actions
Copy link
Contributor

Go solidity wrappers are out-of-date, regenerate them via the make wrappers-all command

1 similar comment
@github-actions GitHub Actions
Copy link
Contributor

Go solidity wrappers are out-of-date, regenerate them via the make wrappers-all command

@github-actions GitHub Actions
Copy link
Contributor

Go solidity wrappers are out-of-date, regenerate them via the make wrappers-all command

@github-actions GitHub Actions
Copy link
Contributor

Go solidity wrappers are out-of-date, regenerate them via the make wrappers-all command

@github-actions GitHub Actions
Copy link
Contributor

Go solidity wrappers are out-of-date, regenerate them via the make wrappers-all command

1 similar comment
@github-actions GitHub Actions
Copy link
Contributor

Go solidity wrappers are out-of-date, regenerate them via the make wrappers-all command

@bolekk bolekk force-pushed the keystone-forwarder branch from fd662f3 to ac2675f Compare February 16, 2024 19:33
@cl-sonarqube-production
Copy link

Quality Gate passed Quality Gate passed

Issues
8 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
83.2% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube

@bolekk bolekk added this pull request to the merge queue Feb 16, 2024
Merged via the queue into develop with commit cce6c80 Feb 16, 2024
107 checks passed
@bolekk bolekk deleted the keystone-forwarder branch February 16, 2024 21:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RensR RensR RensR left review comments

@bolekk bolekk bolekk approved these changes

@DeividasK DeividasK Awaiting requested review from DeividasK

@cedric-cordenier cedric-cordenier Awaiting requested review from cedric-cordenier

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@archseer @bolekk @RensR