Add new fields for externalizing account alias #242

MiroslavGatsanoga · 2022-11-10T11:29:37Z

Signed-off-by: Miroslav Gatsanoga [email protected]

Description:
Creating accounts with EC keys can result in an EVM public address alias being tracked by the consensus nodes. This account identifier should be externalized so that mirror nodes have that information.

Add AccountID.evm_address
Add VirtualAddress message object to capture an evm_address and its default status
Add CryptoCreateTransactionBody.evm_address
Add CryptoGetInfoResponse.virtual_addresses list
Add CryptoUpdateTransactionBody.virtual_address_update
Add TransactionRecord.evm_address

Related issue(s):

Signed-off-by: Miroslav Gatsanoga <[email protected]>

Nana-EC

Initial thoughts reflecting so recent updates

services/crypto_get_info.proto

services/transaction_record.proto

Co-authored-by: Nana Essilfie-Conduah <[email protected]> Signed-off-by: Miroslav Gatsanoga <[email protected]>

Signed-off-by: Miroslav Gatsanoga <[email protected]> Co-authored-by: Nana Essilfie-Conduah <[email protected]>

Signed-off-by: Miroslav Gatsanoga <[email protected]>

tinker-michaelj · 2022-11-16T02:59:06Z

services/basic_types.proto

@@ -110,6 +110,7 @@ message AccountID {
         */
        bytes alias = 4;

+        bytes evmAddress = 5;


We've been using "snake-case" now---so evm_address, is_default_address, etc.

tinker-michaelj · 2022-11-16T03:12:25Z

services/transaction_record.proto

@@ -31,6 +31,7 @@ import "custom_fees.proto";
 import "transaction_receipt.proto";
 import "crypto_transfer.proto";
 import "contract_call_local.proto";
+import "crypto_get_info.proto";


Unneeded import.

tinker-michaelj · 2022-11-16T03:12:44Z

services/crypto_update.proto

@@ -29,6 +29,7 @@ import "basic_types.proto";
 import "duration.proto";
 import "timestamp.proto";
 import "google/protobuf/wrappers.proto";
+import "transaction_record.proto";


⚠️ Unneeded, and causes circular import:

[ERROR] /Users/michaeltinker/Dev/hedera-protobufs-java/src/main/proto/freeze_service.proto [0:0]: query_header.proto:28:1: File recursively imports itself: query_header.proto -> transaction.proto -> transaction_body.proto -> crypto_update.proto -> transaction_record.proto -> contract_call_local.proto -> query_header.proto

tinker-michaelj

Just realized there are some unneeded imports causing circular imports.

Signed-off-by: Dimitar Dinev <[email protected]>

dimitar-dinev · 2022-11-16T15:01:14Z

services/transaction_record.proto

+
+    /**
+     * Virtual address update info, if virtual address is specified in the CryptoUpdate transaction. At most one update can be performed at a time.
+     */
+    VirtualAddressUpdate virtual_address_update = 21;


Won't VirtualAddress be sufficient here?'

Оnly CryptoUpdate can remove virtual addresses, and it has the intended VirtualAddressUpdate in the transaction body. Therefore, if the CryptoUpdate is successful, mirror node can get the needed virtual address update info from the transaction body, and we won't have to duplicate it in the record, correct?

And in all other transactions with virtual address info in the record, we won't be removing the address, but adding it/updating nonce, so the remove field of VirtualAddressUpdate won't be needed -> we can directly use VirtualAddress here.

Good point.
I like the simplification

The VirtualAddressUpdate field can be used in a couple of situations:

When we're initially creating an account. In a couple of use cases, we need to externalize the evmAddress that is calculated on the consensus node side, but a simple bytes calculated_evm_address field will be sufficient. The nonce value is always 0.

When we're updating an account and adding/removing virtual addresses from this account. In this case, the mirror nodes can extract this information from the transaction body and we don't need to write it in the records.

Conclusion: it looks like we can remove the VirtualAddressUpdate field and add bytes calculated_evm_address instead. @steven-sheehy @Nana-EC wdyt?

If we maintain the nonce exposure for the account then simplifying to VirtualAddress would be right.
If we say we don't care and aren't concerned with capturing nonce for ContractCall/ContractCreate cases then sure just capturing the evmAddress is fine as calculated_evm_address.

Btw, why calculated_evm_address vs just evm_address or account_evm_address

Just evm_address is fine

xin-hedera · 2022-11-16T17:10:22Z

services/basic_types.proto

+    /**
+     * Flag if this address is being removed. If false, the virtual address is being added.
+     */
+    google.protobuf.BoolValue removed = 2;


what if removed is not set?

Currently this is for the case where the accounts nonce was incremented and is being exposed in the record stream for mirror node consumption

services/basic_types.proto

shemnon

Outside of the comments other reviewers have requested LGTM, so I'll defer my approvals to them.

Signed-off-by: Miroslav Gatsanoga <[email protected]>

Nana-EC

Looks good. Just had some comment suggestions to make it explicit where the evm address had to be the keccak-256 hash of a ECDSA_SECP256K1 primitive key

services/basic_types.proto

services/crypto_create.proto

tinker-michaelj

Makes sense to me! 👍

Signed-off-by: Miroslav Gatsanoga <[email protected]>

Nana-EC

LGTM

tinker-michaelj

LGTM

Add fields for virtual addresess

00fe6ac

Signed-off-by: Miroslav Gatsanoga <[email protected]>

MiroslavGatsanoga requested review from a team as code owners November 10, 2022 11:29

MiroslavGatsanoga requested review from kimbor and mgoelswirlds November 10, 2022 11:29

Nana-EC requested review from xin-hedera, david-bakin-sl, lukelee-sl, shemnon and steven-sheehy November 10, 2022 16:37

Update proto files to reflect latest HIP updates

80e8dfa

Signed-off-by: Miroslav Gatsanoga <[email protected]>

Nana-EC requested changes Nov 15, 2022

View reviewed changes

services/crypto_get_info.proto Outdated Show resolved Hide resolved

services/crypto_get_info.proto Outdated Show resolved Hide resolved

services/transaction_record.proto Outdated Show resolved Hide resolved

services/transaction_record.proto Outdated Show resolved Hide resolved

Rename virtualAddress field to address

6988e26

Co-authored-by: Nana Essilfie-Conduah <[email protected]> Signed-off-by: Miroslav Gatsanoga <[email protected]>

MiroslavGatsanoga force-pushed the virtual-evm-addresses branch from 0f22707 to 6988e26 Compare November 15, 2022 11:24

MiroslavGatsanoga and others added 2 commits November 15, 2022 13:26

Update remove field type to allow for nulls

6f7cadc

Signed-off-by: Miroslav Gatsanoga <[email protected]> Co-authored-by: Nana Essilfie-Conduah <[email protected]>

Move virtual address types to basic_types

6f8af64

Signed-off-by: Miroslav Gatsanoga <[email protected]>

tinker-michaelj reviewed Nov 16, 2022

View reviewed changes

tinker-michaelj requested changes Nov 16, 2022

View reviewed changes

address PR comments

868da73

Signed-off-by: Dimitar Dinev <[email protected]>

dimitar-dinev reviewed Nov 16, 2022

View reviewed changes

xin-hedera reviewed Nov 16, 2022

View reviewed changes

steven-sheehy requested changes Nov 17, 2022

View reviewed changes

services/basic_types.proto Show resolved Hide resolved

services/basic_types.proto Outdated Show resolved Hide resolved

services/basic_types.proto Outdated Show resolved Hide resolved

services/basic_types.proto Outdated Show resolved Hide resolved

stoqnkpL reviewed Nov 18, 2022

View reviewed changes

services/basic_types.proto Outdated Show resolved Hide resolved

shemnon reviewed Nov 18, 2022

View reviewed changes

MiroslavGatsanoga added 3 commits November 22, 2022 13:08

Update to latest agreed version

521b9a9

Signed-off-by: Miroslav Gatsanoga <[email protected]>

Remove unneeded VirtualAddressUpdate type

c2006b7

Signed-off-by: Miroslav Gatsanoga <[email protected]>

Update docs

f423962

Signed-off-by: Miroslav Gatsanoga <[email protected]>

MiroslavGatsanoga removed the request for review from kimbor November 22, 2022 15:38

MiroslavGatsanoga requested review from tinker-michaelj, steven-sheehy and Nana-EC and removed request for steven-sheehy and tinker-michaelj November 22, 2022 15:38

Nana-EC previously approved these changes Nov 22, 2022

View reviewed changes

services/basic_types.proto Outdated Show resolved Hide resolved

services/basic_types.proto Outdated Show resolved Hide resolved

services/crypto_create.proto Outdated Show resolved Hide resolved

steven-sheehy previously approved these changes Nov 22, 2022

View reviewed changes

tinker-michaelj reviewed Nov 22, 2022

View reviewed changes

Improve docs around EVM address

e676c94

Signed-off-by: Miroslav Gatsanoga <[email protected]>

MiroslavGatsanoga dismissed stale reviews from steven-sheehy and Nana-EC via e676c94 November 23, 2022 09:38

MiroslavGatsanoga requested review from tinker-michaelj and Nana-EC and removed request for tinker-michaelj and Nana-EC November 23, 2022 09:39

Nana-EC approved these changes Nov 23, 2022

View reviewed changes

tinker-michaelj approved these changes Nov 23, 2022

View reviewed changes

steven-sheehy approved these changes Nov 23, 2022

View reviewed changes

Nana-EC assigned MiroslavGatsanoga Nov 23, 2022

Nana-EC merged commit 44f0afe into hashgraph:main Nov 23, 2022

MiroslavGatsanoga deleted the virtual-evm-addresses branch November 24, 2022 08:07

lukelee-sl mentioned this pull request Feb 8, 2023

Remove the idea of virtual addresses and replace with alias(es) #259

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new fields for externalizing account alias #242

Add new fields for externalizing account alias #242

MiroslavGatsanoga commented Nov 10, 2022 •

edited by Nana-EC

Loading

Nana-EC left a comment

tinker-michaelj Nov 16, 2022

dimitar-dinev Nov 16, 2022

tinker-michaelj Nov 16, 2022

dimitar-dinev Nov 16, 2022

tinker-michaelj Nov 16, 2022 •

edited

Loading

dimitar-dinev Nov 16, 2022

tinker-michaelj left a comment

dimitar-dinev Nov 16, 2022

Nana-EC Nov 16, 2022

stoqnkpL Nov 18, 2022

Nana-EC Nov 18, 2022

Nana-EC Nov 22, 2022

MiroslavGatsanoga Nov 22, 2022

xin-hedera Nov 16, 2022

Nana-EC Nov 18, 2022

shemnon left a comment

Nana-EC left a comment

tinker-michaelj left a comment

Nana-EC left a comment

tinker-michaelj left a comment

Add new fields for externalizing account alias #242

Add new fields for externalizing account alias #242

Conversation

MiroslavGatsanoga commented Nov 10, 2022 • edited by Nana-EC Loading

Nana-EC left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tinker-michaelj Nov 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tinker-michaelj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shemnon left a comment

Choose a reason for hiding this comment

Nana-EC left a comment

Choose a reason for hiding this comment

tinker-michaelj left a comment

Choose a reason for hiding this comment

Nana-EC left a comment

Choose a reason for hiding this comment

tinker-michaelj left a comment

Choose a reason for hiding this comment

MiroslavGatsanoga commented Nov 10, 2022 •

edited by Nana-EC

Loading

tinker-michaelj Nov 16, 2022 •

edited

Loading