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

Add virtual_address_update disable #249

Closed
wants to merge 4 commits into from
Closed

Add virtual_address_update disable #249

wants to merge 4 commits into from

Conversation

Nana-EC
Copy link
Contributor

@Nana-EC Nana-EC commented Dec 6, 2022

Signed-off-by: Nana Essilfie-Conduah [email protected]

Description:
To support the ability to disable a virtual address but keep it associated with an account

  • add disable to CryptoUpdateTransactionBody.virtual_address_update

Related issue(s):

Fixes #248

Notes for reviewer:

Checklist

  • Documented (Code comments, README, etc.)
  • Tested (unit, integration, etc.)

@Nana-EC
Add virtual_address_update disable
4e37749 
Signed-off-by: Nana Essilfie-Conduah <[email protected]>
@Nana-EC Nana-EC self-assigned this Dec 6, 2022
@Nana-EC Nana-EC added the enhancement New feature or request label Dec 6, 2022
@Nana-EC Nana-EC marked this pull request as ready for review December 6, 2022 05:25
@Nana-EC Nana-EC requested review from a team as code owners December 6, 2022 05:25
steven-sheehy
steven-sheehy requested changes Dec 6, 2022
View reviewed changes
services/crypto_update.proto Outdated
bytes remove = 21;
* The 20-byte EVM address of the virtual address that is being disabled and therefore removed from active use on the account.
*/
bytes disable = 21;
Copy link
Member

Choose a reason for hiding this comment

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

Since a disabled address stays in state, shouldn't this flag be moved to VirtualAddress? If queries only return active addresses how would users know which old, inactive addresses they need to delete?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah good suggestion. Will leave till we're ready to turn on disable

services/crypto_update.proto Outdated
* The 20-byte EVM address of the virtual address that is being removed.
*/
bytes remove = 21;
* The 20-byte EVM address of the virtual address that is being disabled and therefore removed from active use on the account.
Copy link
Member

Choose a reason for hiding this comment

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

The only difference between disable and remove is that the former retains ownership and the latter allows reuse. Add a comment to each field about the impacts of address reuse.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do when we circle back

services/crypto_update.proto Outdated
bytes remove = 21;
* The 20-byte EVM address of the virtual address that is being disabled and therefore removed from active use on the account.
*/
bytes disable = 21;
Copy link
Member

@steven-sheehy steven-sheehy Dec 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Don't think we should add things to proto that won't be implemented in the next release. We tend to do that and never implement it or want to implement it differently.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's fair.
We'll coordinate the disable addition.
Will remove as suggested since adding to a oneof later on (even if the ordinal value is greater than existing outer fields) won't break the API

@Nana-EC
remove disable and simplify
b3b7b75 
Signed-off-by: Nana Essilfie-Conduah <[email protected]>
steven-sheehy
steven-sheehy reviewed Dec 6, 2022
View reviewed changes
services/crypto_update.proto Outdated
@@ -167,17 +167,8 @@ message CryptoUpdateTransactionBody {
oneof virtual_address_update {
/**
* The virtual address to be added.
* The address one successfully addition becomes par tof the accounts ACTIVE virtual addresses
Copy link
Member

Choose a reason for hiding this comment

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

nit: part of

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, thanks

@Nana-EC
fixed broken part of
2e78f38 
Signed-off-by: Nana Essilfie-Conduah <[email protected]>
steven-sheehy
steven-sheehy previously approved these changes Dec 7, 2022
View reviewed changes
kimbor
kimbor reviewed Dec 9, 2022
View reviewed changes
services/crypto_update.proto Outdated
@@ -167,7 +167,7 @@ message CryptoUpdateTransactionBody {
oneof virtual_address_update {
/**
* The virtual address to be added.
* The address one successfully addition becomes par tof the accounts ACTIVE virtual addresses
* The address one successfully addition becomes part of the accounts ACTIVE virtual addresses
Copy link
Member

Choose a reason for hiding this comment

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

Once successfully added, the address becomes part of the account's ACTIVE virtual addresses.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kimbor fixed

@Nana-EC
fixed comment
f2dbe8b 
Signed-off-by: Nana Essilfie-Conduah <[email protected]>
MarcKriguerAtHedera
MarcKriguerAtHedera approved these changes Jan 21, 2023
View reviewed changes
Copy link

@MarcKriguerAtHedera MarcKriguerAtHedera left a comment

Choose a reason for hiding this comment

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

Despite the PR being called "Add virtual_address_update disable", it's really removing the remove action. If that's what is intended, this little change looks good to me.

@Nana-EC
Copy link
Contributor Author

Nana-EC commented Feb 1, 2023

Despite the PR being called "Add virtual_address_update disable", it's really removing the remove action. If that's what is intended, this little change looks good to me.

Yes, will move this into draft.
The final form will see remove replaced with disable

@Nana-EC Nana-EC marked this pull request as draft February 1, 2023 13:24
@david-bakin-sl
Copy link
Member

So what's here now is removing the field remove because it is not yet implemented? If so the title and main comment of this PR should be changed to match?

@Nana-EC
Copy link
Contributor Author

Nana-EC commented Feb 27, 2023

Closing until HIP 631 is revisited with HIP 583 induced changes

@Nana-EC Nana-EC closed this Feb 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MarcKriguerAtHedera MarcKriguerAtHedera MarcKriguerAtHedera approved these changes

@david-bakin-sl david-bakin-sl Awaiting requested review from david-bakin-sl

@lukelee-sl lukelee-sl Awaiting requested review from lukelee-sl

@shemnon shemnon Awaiting requested review from shemnon

@mhess-swl mhess-swl Awaiting requested review from mhess-swl mhess-swl is a code owner automatically assigned from hashgraph/hedera-services

@steven-sheehy steven-sheehy Awaiting requested review from steven-sheehy

@kimbor kimbor Awaiting requested review from kimbor

@tinker-michaelj tinker-michaelj Awaiting requested review from tinker-michaelj

@Neeharika-Sompalli Neeharika-Sompalli Awaiting requested review from Neeharika-Sompalli

Assignees

@Nana-EC Nana-EC

Labels
enhancement New feature or request
Projects
Smart Contract Sprint Board
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update virtual_address_update with diable logic
5 participants
@Nana-EC @david-bakin-sl @kimbor @steven-sheehy @MarcKriguerAtHedera