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 4 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.
I think the context is still valuable. A reader of a HIP should be able to read the Abstract and get a general idea of the sections of the spec, for discussion.
I adopted the 1 liner to give the answer to"What" but preserved the 2 short paragraphs to allow the idea to be presented.
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.
I still don't understand why the HIP allows them to set an ECDSA derived EVM address on their existing account but then it doesn't allow them to use this new value on contract calls and instead requires the long zero form.
And even if Hedera Num Alias is required, why would they specify it as long zero bytes instead of just using
AccountID
in shard.realm.num?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.
If a valid alias is set where one did not exist the new value is indeed used during Contract calls.
The override option is provided to set the older long-zero address in the case that a user would like to be perceived by the EVM as such this to unblock themselves.
This is why the address format is used and not the AccountID format.
For example, say an account with no alias has interacted with a contract that cached its long zero address. If they set the alias with this HIP and again interacted with the contract they would likely see the new address cached.
If for some reason that account wanted to migrate in contract details from the original long zero address to the new ECDSA derived address they could do so by utilizing this override in addition to the contract logic.
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.
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.
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.
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.
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