-
Notifications
You must be signed in to change notification settings - Fork 15
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 #1698
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 063f5e1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
enumerating various test cases manually per #1660 (comment). The way the logic is structured, if both 0. User sends the non-maximum amount of an alternative fee asset (GM), and pays fees with the native asset (UM).
1. User sends the maximum amount of an alternative fee asset (GM), and pays fees with the native asset (UM).
2. User sends the non-maximum amount of an another asset (Pizza), and pays fees with an alternative fee asset (GN).
3. User sends the maximum amount of an another asset (Pizza), and pays fees with an alternative fee asset (GN).
4. User sends the non-maximum amount of the native asset (UM).
5. User sends the maximum amount of the native asset (UM).
6. User sends the non-maximum amount of alternative asset (GM), and pays fees with the same alternative asset (GM) since the native token (UM) is absent.
7. User sends the maximum amount of alternative asset (GM), and pays fees with the same alternative asset (GM) since the native token (UM) is absent.
8. Not able to pay for fees." |
@Valentine1898 optimistically requested review. I'll implement more comprehensive unit testing and incorporate the additional constraint checks. |
apps/minifront/src/components/shared/non-native-fee-warning.tsx
Outdated
Show resolved
Hide resolved
* refactor to use zustand and remove extraneous state * more refactor and linting
update this has taken a backseat in favor of more pressing sync refactoring, but reprioritizing this by adding unit tests that have been sitting in a local branch for some time. |
waiting for runner to pick up CI job. @penumbra-zone/web-team please test with sample transactions for extra assurance folks! |
references #1660 and depends on #1801.
to address the feature regression, we implement additional safety checks in both the frontend and services package.
The frontend uses checkSendMaxInvariants to determine if certain invariants are satisfied and decide whether to construct the transaction as a
Spend
orOutput
transaction planner request. This determines the appropriate planner request that the wasm planner will consume to craft the fully-formed transaction.In the transaction planner service implementation, we've added additional constraint checks in assertSpendMax on the fully-formed transaction. However, these checks occur only after the planning step because, with the current code, there's no effective way to enforce these constraints directly in the wasm planner. This provides a greater degree of validation and safety.
In thinking about this more, while these approaches increase safety, they don't completely cover the entire domain space. There is still an external risk to third-party consumers of our wasm package since we don't enforce these checks directly in the wasm planner. To address this, the final step is to implement the same checks in
assertSpendMax
directly within the wasm planner. This requires public getters to access the private fields inside the ActionList struct. This is currently implemented in this PR but is blocked until a minor point release is cut on the protocol side.Update – separated the wasm code into #1801 and added additional safety checks directly within the wasm planner, along with wasm-bindgen tests.