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

Re-enable spend max feature #1660

Closed
TalDerei opened this issue Aug 8, 2024 · 7 comments
Closed

Re-enable spend max feature #1660

TalDerei opened this issue Aug 8, 2024 · 7 comments
Assignees
Labels
bug Something isn't working refactor Improving existing system with new design security wasm Rust crate updates

Comments

@TalDerei
Copy link
Contributor

TalDerei commented Aug 8, 2024

Context

#1612 and #1611 resulted in a feature regression that should be re-enabled and properly implemented. Below is a high-level product plan for achieving this.

Updates required to enable send max

  • check user flows
    • test a set of user flows associated with the feature to ensure all use cases are covered
  • enable the feature in the transaction planner request within minifront
  • fix the feature in the wasm planner
    • ensure that the remaining balance from the fee note is returned to the originating address of the transaction

cc @grod220

@TalDerei TalDerei added bug Something isn't working refactor Improving existing system with new design wasm Rust crate updates labels Aug 8, 2024
@github-project-automation github-project-automation bot moved this to 🗄️ Backlog in Penumbra web Aug 8, 2024
@TalDerei
Copy link
Contributor Author

TalDerei commented Aug 14, 2024

Currently, we leverage our multi-fee system to accidentally support maximum sends in certain scenarios. Consider the following cases:


Active Functionality:

These are constructed using an output transaction planner request (TPR).

1. Sending an alternative asset (GM) while paying fees with the native asset (UM).

Screenshot 2024-08-14 at 8 47 55 AM Screenshot 2024-08-14 at 8 48 38 AM

2. Sending an alternative asset (Pizza) while paying fees with another alternative asset (GN).

Screenshot 2024-08-14 at 8 48 19 AM

Inactive Functionality:

These would be constructed using an spend transaction planner request (TPR).

3. Sending the maximum amount of the native asset (UM).
4. Sending the maximum amount of an alternative asset (GM) without any native asset (UM) balance, and paying the fee with the same alternative asset (GM).


Proposals:

Invariants (3) and (4) fundamentally both rely on the presence of UM. We need to handle these cases by checking the following conditions:

  • If the user sends the maximum amount of the native asset (UM):
    • craft a spend TPR.
  • If the user sends max alternative asset (GM), and UM is absent:
    • craft a spend TPR.
  • if the user sends another asset (Pizza), and UM is absent
    • fall back to the multi-fee system and craft an output TPR to ensure the fees are handled correctly.
  • If the user sends the maximum amount of an alternative asset (GM) and the native asset (UM) is not absent:
    • craft output TPR.

Alternatively, the spend TPR can be reserved exclusively for the native asset (UM). For all other alternative assets, when sending the maximum amount, the output TPR can be used, leveraging our multi-fee system to handle the transaction efficiently.

cc @grod220 @Valentine1898 wondering your thoughts?

@github-project-automation github-project-automation bot moved this to 🗄️ Backlog in Labs web Aug 14, 2024
@TalDerei TalDerei self-assigned this Aug 14, 2024
@Valentine1898
Copy link
Contributor

Valentine1898 commented Aug 14, 2024

@TalDerei I believe your proposed solution will work

But to me the important point is that the planner should reject TPR that contains spend actions in these cases:

  1. If spend.asset_id != fee.asset_id
  2. If spend amount is not equal to the maximum amount
    This also leads to a secondary constraint:
  3. TPR cannot contain more than one spend action

I believe that these restrictions should be added specifically to the planner so that client-side mistakes don't create unexpected TransactionPlans

@TalDerei
Copy link
Contributor Author

But to me the important point is that the planner should reject TPR that contains spend actions in these cases:

Just to clarify, are you leaning towards checking the aforementioned conditions on the frontend, or do you prefer only supporting max sends for UM? The former is more flexible.

@Valentine1898
Copy link
Contributor

But to me the important point is that the planner should reject TPR that contains spend actions in these cases:

Just to clarify, are you leaning towards checking the aforementioned conditions on the frontend, or do you prefer only supporting max sends for UM? The former is more flexible.

I think we should support MAX send not only for UM, but for alt_fee tokens as well.
But my argument is that we should also change the code of our RPC planner and add these conditions, not just change the TPR construction logic on the frontend

@TalDerei
Copy link
Contributor Author

TalDerei commented Aug 14, 2024

But my argument is that we should also change the code of our RPC planner and add these conditions, not just change the TPR construction logic on the frontend

Agreed. On another note, it's unclear to me how to support max spends for alternative fee tokens, like GM/ GN. Consider bullet points 2 and 3 in the proposal. Is there a way to distinguish what is a valid alternative fee token, compared to a non-valid fee token like TestUSD, in the frontend? Typically, we rely on querying the database for this information.

@Valentine1898
Copy link
Contributor

I don't think there's a way to distinguish what is a valid alternative fee token, compared to a non-valid fee token like TestUSD, in the frontend? Typically, we rely on querying the database for this information.

The frontend can query gas prices RPC and get a list of alt fee tokens
We already do this to display alt fee warning correctly, see #1614.

@TalDerei
Copy link
Contributor Author

TalDerei commented Aug 14, 2024

But to me the important point is that the planner should reject TPR that contains spend actions in these cases:

we can't directly query the actions contained in the ActionList since it's not a public field, and exposing those actions with a getter doesn't seem preferable. Alternatively, we could consider adding the checks in the transaction planner service implementation before returning the fully-formed TransactionPlan. This way, even if the planner somehow ends up planning an invalid transaction, it wouldn't make its way back to the frontend.

@TalDerei TalDerei moved this from 🗄️ Backlog to 📝 Todo in Labs web Aug 15, 2024
@TalDerei TalDerei moved this from 📝 Todo to 🏗 In progress in Labs web Aug 15, 2024
@github-project-automation github-project-automation bot moved this from 🗄️ Backlog to ✅ Done in Penumbra web Oct 30, 2024
@github-project-automation github-project-automation bot moved this from 🔎 In review to ✅ Done in Labs web Oct 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working refactor Improving existing system with new design security wasm Rust crate updates
Projects
Archived in project
Development

No branches or pull requests

2 participants