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.

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

pcli: add balance migration command #4842

Merged
merged 1 commit into from
Sep 5, 2024
Merged

pcli: add balance migration command #4842

merged 1 commit into from
Sep 5, 2024

Conversation

aubrika
Copy link
Contributor

@aubrika aubrika commented Aug 28, 2024

initial attempt at implementing a balance migration function for pcli - does not currently handle non-zero fees correctly, but that seems to be the only remaining thing to correctly implement

  • If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason:

    client/planner only changes

@aubrika
Copy link
Contributor Author

aubrika commented Aug 29, 2024

Conor and I confirmed that this implementation seems to work, but only by doubling the estimated fee. Work on this has revealed some issues:

  1. The fee estimation logic in the Planner was known to be approximate, but simply doubling the expected fee is an inelegant way to ensure that sufficient funds remain to pay the fees. Can we make this estimation more accurate?

  2. The Planner itself is designed such that under its default behavior, it is not possible to easily send a "maximum" amount of the fee token - attempting to send 100% of the fee token will result in a failure, and requires some arithmetic on the part of the user to infer how much can actually be sent. Should we refactor the Planner to perform this calculation automatically, and subtract the fees from the intended amount to send, rather than adding fees to the intended amount to send?

@cronokirby
Copy link
Contributor

I think we want to avoid having to work around the planner, and should instead have a way to express to the planner the desire to send the maximum amount.

@conorsch
Copy link
Contributor

conorsch commented Sep 4, 2024

Tested @cronokirby's additions locally, and very satisfied with the results. The source wallet was completely emptied, with no dust notes remaining. The destination wallet received all amounts of all assets, minus the small fee paid to make the transaction. Using the change address was a big missing piece in the first draft of the implementation.

Now, the only concerns I have remaining are:

  1. The FVK should be read from stdin, not on the CLI, given that it's sensitive info and shouldn't be memorized in one's shell history forever.
  2. It'd be grand to have a functional test for this behavior, but I view that as a nice-to-have.

Regarding 1, I realize we commit the sin of asking for FVK as a CLI opt elsewhere, too, but would like to upgrade the experience to minimize account info dangling in insecure contexts.

@cronokirby
Copy link
Contributor

Tested @cronokirby's additions locally, and very satisfied with the results. The source wallet was completely emptied, with no dust notes remaining. The destination wallet received all amounts of all assets, minus the small fee paid to make the transaction. Using the change address was a big missing piece in the first draft of the implementation.

Now, the only concerns I have remaining are:

1. The FVK should be read from stdin, not on the CLI, given that it's sensitive info and shouldn't be memorized in one's shell history forever.

2. It'd be grand to have a functional test for this behavior, but I view that as a nice-to-have.

Regarding 1, I realize we commit the sin of asking for FVK as a CLI opt elsewhere, too, but would like to upgrade the experience to minimize account info dangling in insecure contexts.

Point 1 addressed.

@conorsch conorsch changed the title WIP: pcli: add balance migration command pcli: add balance migration command Sep 5, 2024
@conorsch
Copy link
Contributor

conorsch commented Sep 5, 2024

Absolutely awesome. The stdin reading prompts interactively for the FVK, and supports reading from pipes, so it's still scriptable, but the FVK won't ever show up as a cli arg in terminal history. Tacked on some nice-to-haves like a custom error message for an empty balance (happens if you try to migrate a second time on the same source wallet), and more verbose help docs. Will clean up the submission text and squash-merge to land.

@conorsch conorsch self-requested a review September 5, 2024 17:43
Adds a new subcommand to pcli for moving the entirety of a wallet's
assets to a new wallet, as defined by an FVK. Stores both FVKs in the
memo of the transaction. Leaves zero notes in the source wallet,
and the dest wallet gets everything, minus whatever gas price was paid.

The full-send functionality is achieved by using the change address
of the planner, setting it to the dest FVK's 0 address.
This approach avoids the issue of dealing with fees outside the planner.
It does this by eschewing trying to figure out how much to output,
and instead focusing on what to spend (everything).
Then, the change address is set to the desired destination, allowing the
planner to automatically move the funds as needed, minus fees.

Ensures that the FVK is read from stdin, so that it's never placed on
the command line.

Co-authored-by: aubrey <[email protected]>
Co-authored-by: Lucas Meier <[email protected]>
Co-authored-by: Conor Schaefer <[email protected]>
@cronokirby cronokirby merged commit 9443d98 into main Sep 5, 2024
13 checks passed
@cronokirby cronokirby deleted the 4831-pcli-migrate branch September 5, 2024 18:10
@conorsch conorsch mentioned this pull request Sep 5, 2024
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants