-
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] COA ownership proof - part 3 #5343
Conversation
signedData: [UInt8], | ||
keyIndices: [UInt64], | ||
signatures: [[UInt8]], | ||
evmAddress: [UInt8; 20] |
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.
random thought, but I'm seeing this array definition multiple times and I think we should maybe create an evmAddress
type for it
@@ -3646,3 +3648,155 @@ func TestEVMAccountBalanceForABIOnlyContract(t *testing.T) { | |||
"error: value of type `EVM` has no member `createBridgedAccount`", | |||
) | |||
} | |||
|
|||
func TestEVMValidateCOAOwnershipProof(t *testing.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.
Could we also add a test where the proof fails
fvm/evm/testutils/contract.go
Outdated
require.NoError(tb, err) | ||
return &TestContract{ | ||
Code: ` | ||
pragma solidity >=0.7.0 <0.9.0; | ||
// SPDX-License-Identifier: GPL-3.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.
should we maybe start extracting solidity code and bytecode into separate files, like Storage.sol
which are then embeded into the Go code using the embed
. I prefer it because you have syntax highlighting possible, you can use the file directly to compile to bytecode, I even think we should in the future generate the bytecode file using a make action, same for ABI, have ABI.json and that could also get generated in the future.
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 looks good
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.
👍
@@ -61,6 +66,23 @@ func NewAddressFromBytes(inp []byte) Address { | |||
return Address(gethCommon.BytesToAddress(inp)) | |||
} | |||
|
|||
func COAAddressFromFlowEvent(evmContractAddress flow.Address, event flow.Event) (Address, error) { | |||
// check the type first | |||
if string(event.Type) != fmt.Sprintf(COAAddressTemplate, evmContractAddress.Hex()) { |
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.
Maybe optimize this check by avoiding the string formatting/allocation, and just decode the event's type ID and comparing address and qualified identifier
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 is not a code that is touched by the onchain execution but I'll update the next PR to do as you suggested. How do I decode the event type ID? is there a utility function on the cadence side to decode from string?
…into ramtin/5197-part3-stdlib
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #5343 +/- ##
==========================================
- Coverage 56.02% 55.91% -0.12%
==========================================
Files 965 1024 +59
Lines 89671 98842 +9171
==========================================
+ Hits 50241 55268 +5027
- Misses 35678 39403 +3725
- Partials 3752 4171 +419
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
This updates the evm contract with
verify
function for COA ownership proofs, it also updates the handler to use theinvoke
to make a call.closes #5197