Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Allow Setting of EVM Address by EOA #1082
base: main
Are you sure you want to change the base?
Allow Setting of EVM Address by EOA #1082
Changes from 3 commits
226d5b1
fe1a3c1
ef50d53
6ab73e0
8f3f51b
b1648e1
32b20dc
0074a38
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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.
Adopted change
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.
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.
Adopted change
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.
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.
Adopted change
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.
In this user story, wouldn't the user have to update the ED key to the new ECDSA key that corresponds with the EVM address for 'ecrecover' to function? Maybe I'm misunderstanding the phrase 'correct ecrecover functionality', or is the implication that the key is being updated as well
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.
No, the user would just use their ECDSA key in a wallet to sign. So far as the EVM address can be extracted from the signature the ecercover flow would work
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.
Is this out of scope / too complex for this HIPs user story?
As an existing account with an ED key but no EVM address alias, I would like to update my key to an ECDSA key and set my EVM address to be derived from the new ECDSA key.
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.
Out of scope, not too complex but would be enabled by this HIP and current network functionality.
Such a user could rotate their keys before or after setting the EVM address.
They could actually do it in one single CryptoUpdate also.
Key rotation is not tied to alias value
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 didn't think it was out of scope, but explicitly in scope. Key rotation is already supported as you said, and setting the alias if it was null is also in scope. So what @ty-swirldslabs is asking for should be a natural consequence of this HIP?
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 read this as a User story to 1) rotate keys 2) set new ECDSA derived evm address.
Valid point though so I've added a new user story to highlight the chaining of actions as this is valuable for users.
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.
Is this resolved @ty-swirldslabs @Nana-EC ?
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.
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.
Adopted change
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.
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.
Adopted change
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 HIP requires two changes to consensus nodes:
CryptoUpdate
transactions so allow the alias to be set to an ECDSA derived address if the aliashas never been set.
ContractCall
to support an "EVM address override", so the long-zero or ECDSA derive alias maybe explicitly used.
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.
Adopted change
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.
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.
Adopted change
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 name seems ambiguous to me. It could be the address of the contract or the sender. What about
from
like ineth_call
? Or other alternatives likefrom_address
orsender
?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.
Fair point.
sender
might be cleaner as in implementation this is setting the EOAmsg.sender
observed by the EVMThere 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.
We have both
evm_address
andalias
in theTransactionRecord
. The documentation forevm_address
is suspicious:I'm not exactly sure what that means. We should double check on whether we write the evm_address, alias, or both in the transaction record after an update. And we should move this comment into the previous section for
CryptoUpdateTransactionBody
.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.
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.
Adopted change
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 doesn't mention changes around processing contract call and create.
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.
The CN should ensure that the override address is adopted fully by the EVM, so there should be no changes to Mirror Node processing on those calls.
The
sender
value inContractFunctionResult
should be that of the override utilized in the transaction body.Do you see any processing changes as a result of this or in other areas?
I'm happy to call them out or address them if they were missed
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 we detail any API changes? My assumption that there's no new fields but that existing fields in
/api/v1/accounts/{id}
will show the updated evm_address and the existingfrom
in contract result APIs will show the override?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.
Correct, the
evm_address
for an account will show the new value.New contract results would also show the override.
Older accounts values and contract results should show the long zero if that's what was in the record file for correctness.
As such there shouldn't be any API field changes.
DO you see otherwise?
Added some details for this
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.
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.
Adopted change
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.
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.
Adopted change
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.
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.
Adopted change
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.
Is this true? I thought 631 allowed having multiple aliases, and thus would support going from null to an alias as well. The only problem with 631 was that it was complicated by having multiple aliases, and we decided to try a simpler approach with just 1 alias (in addition to the long-zero).
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.
HIP 631 specifically reserved a different space in state for the multiple virtual addresses but maintained a single account.alias field as immutable
Pulled from HIP 631, also the account diagram notes virtual addresses as a different field altogether. We never stated the ability to set a the null alias, so even with multiple virtual addresses an account could still have a null alias