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

wallet: obtain subnet owners by using P-Chain's getSubnet API call #3247

Merged
merged 17 commits into from
Aug 14, 2024
Merged

wallet: obtain subnet owners by using P-Chain's getSubnet API call #3247

merged 17 commits into from
Aug 14, 2024

Conversation

felipemadero
Copy link
Contributor

Why this should be merged

currently the wallet is expected to receive as params the latest transform subnet ownership tx or tx id for the subnet,
so as to be able to obtain the current owner.
this is not needed as current owner can be obtained by calling P-Chain's getSubnet API call

How this works

it makes wallet's P-Chain backed to receive a map of subnet ids to owners.
on MakeWallet, the map is generated from the txs and tx ids in wallet config

How this was tested

it was tested using CLI command for changing subnet owners

This was referenced Jul 30, 2024
Merged
Merged
@felipemadero felipemadero self-assigned this Jul 30, 2024
marun
marun reviewed Jul 31, 2024
View reviewed changes
Copy link
Contributor

@marun marun left a comment

Choose a reason for hiding this comment

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

Maybe add test coverage (unit or e2e) for the new functionality to minimize the potential for changes to avalanchego to unintentionally break CLI?

wallet/subnet/primary/wallet.go Outdated Show resolved Hide resolved
wallet/subnet/primary/wallet.go Outdated Show resolved Hide resolved
wallet/subnet/primary/wallet.go Outdated Show resolved Hide resolved
@felipemadero
Copy link
Contributor Author

felipemadero commented Aug 6, 2024 •
edited
Loading

Maybe add test coverage (unit or e2e) for the new functionality to minimize the potential for changes to avalanchego to unintentionally break CLI?

Best option seems to be to add an e2e for TransferSubnetOwnershipTx but that seems outside the scope of this PR.
Not issue in creating an e2e in a separate PR but would like to agree with you on the logic to use and where to put it.

@felipemadero felipemadero requested a review from marun August 6, 2024 00:33
@marun
Copy link
Contributor

marun commented Aug 6, 2024

Best option seems to be to add an e2e for TransferSubnetOwnershipTx but that seems outside the scope of this PR. Not issue in creating an e2e in a separate PR but would like to agree with you on the logic to use and where to put it.

Given that there is otherwise no coverage of the changes to wallet.go (unlike the unit-tested builder), I think it would be appropriate to include an e2e test in this PR. I'd be open to having the changes to the wallet extracted and trivially e2e-tested (i.e. create subnet, verify that new function returns the expected owners) if validation of usage in transaction handling seems unreasonable.

sukantoraymond
sukantoraymond approved these changes Aug 6, 2024
View reviewed changes
Copy link
Contributor

@sukantoraymond sukantoraymond left a comment

Choose a reason for hiding this comment

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

LGTM

marun
marun previously requested changes Aug 6, 2024
View reviewed changes
Copy link
Contributor

@marun marun left a comment

Choose a reason for hiding this comment

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

Please ensure e2e test coverage of the wallet change pre-merge.

@marun marun dismissed their stale review August 6, 2024 15:30

My apologies, I'll leave this to Stephen.

@felipemadero felipemadero requested a review from marun August 9, 2024 15:27
@felipemadero
Copy link
Contributor Author

Please ensure e2e test coverage of the wallet change pre-merge.

refactored into separate function + added e2e @marun @StephenButtolph

marun
marun reviewed Aug 11, 2024
View reviewed changes
Copy link
Contributor

@marun marun left a comment

Choose a reason for hiding this comment

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

Apologies for the delay!

tests/e2e/p/owner_retrieval.go Outdated Show resolved Hide resolved
tests/e2e/p/owner_retrieval.go Outdated Show resolved Hide resolved
tests/e2e/p/owner_retrieval.go Outdated Show resolved Hide resolved
tests/e2e/p/owner_retrieval.go Outdated Show resolved Hide resolved
wallet/subnet/primary/wallet.go Show resolved Hide resolved
wallet/subnet/primary/wallet.go Outdated Show resolved Hide resolved
tests/e2e/p/owner_retrieval.go Outdated Show resolved Hide resolved
tests/e2e/p/owner_retrieval.go Outdated Show resolved Hide resolved
StephenButtolph
StephenButtolph reviewed Aug 13, 2024
View reviewed changes
wallet/subnet/primary/wallet.go Outdated Show resolved Hide resolved
wallet/subnet/primary/helpers.go Outdated Show resolved Hide resolved
marun
marun approved these changes Aug 13, 2024
View reviewed changes
Copy link
Contributor

@marun marun left a comment

Choose a reason for hiding this comment

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

Nice cleanup!

tests/e2e/p/owner_retrieval.go Outdated Show resolved Hide resolved
@StephenButtolph StephenButtolph added this pull request to the merge queue Aug 14, 2024
Merged via the queue into master with commit 208eff7 Aug 14, 2024
21 checks passed
@StephenButtolph StephenButtolph deleted the make-wallet-to-use-getsubnet branch August 14, 2024 01:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sukantoraymond sukantoraymond sukantoraymond approved these changes

@StephenButtolph StephenButtolph StephenButtolph approved these changes

@marun marun marun approved these changes

@abi87 abi87 Awaiting requested review from abi87

Assignees

@felipemadero felipemadero

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@felipemadero @marun @StephenButtolph @sukantoraymond