Skip to content
This repository has been archived by the owner on Oct 6, 2023. It is now read-only.

Update AccountsStrategy._payForGasWithAccountBalance to accept gas percentage to pay from liquid balance #289

Closed
wants to merge 7 commits into from

Conversation

0xNeshi
Copy link
Contributor

@0xNeshi 0xNeshi commented Aug 8, 2023

Explanation of the solution

When calling strategyRedeem(All) funcs and having to pay part of gas fees from Accounts Diamond itself, we currently pass value 1 for both lockAmt and liqAmt to _payForGasWithAccountBalance to force it to split the fee equally between locked and liquid balance portions. This in turn, with the current logic, causes balances to be decreased not just by the necessary gas amount(s), but also by this 1 extra token on both locked and liquid.

To address this, we update _payForGasWithAccountBalance to know how to split gas fees when either or both lockAmt and liqAmt are 0 (zero). to accept a gasPercentFromLiq parameter indicating the percentage of gas to be payed from liquid balance, with the rest being payed from locked balance.
This way we make it possible to preserve existing behavior, which is to make fee split proportional on invest and redeem, but split in half on redeemAll, while also removing the unintended behavior described above.

Instructions on making this work

  • run yarn or yarn install to install npm dependencies

@0xNeshi 0xNeshi added the bug Something isn't working label Aug 8, 2023
@0xNeshi 0xNeshi self-assigned this Aug 8, 2023
@0xNeshi
Copy link
Contributor Author

0xNeshi commented Aug 8, 2023

cc: @SovereignAndrey @stevieraykatz

0xNeshi and others added 2 commits August 9, 2023 07:08
Co-authored-by: katzman <84420280+stevieraykatz@users.noreply.github.com>
@0xNeshi 0xNeshi changed the title Pass 0 tokens to AccountsStrategy._payForGasWithAccountBalance on redeem func calls Update AccountsStrategy._payForGasWithAccountBalance to accept gas percentage to pay from liquid balance Aug 9, 2023
@0xNeshi 0xNeshi changed the title Update AccountsStrategy._payForGasWithAccountBalance to accept gas percentage to pay from liquid balance [WIP] Update AccountsStrategy._payForGasWithAccountBalance to accept gas percentage to pay from liquid balance Aug 9, 2023
@0xNeshi 0xNeshi added the WIP label Aug 9, 2023
@0xNeshi 0xNeshi removed the WIP label Aug 9, 2023
@0xNeshi 0xNeshi changed the title [WIP] Update AccountsStrategy._payForGasWithAccountBalance to accept gas percentage to pay from liquid balance Update AccountsStrategy._payForGasWithAccountBalance to accept gas percentage to pay from liquid balance Aug 9, 2023
@0xNeshi
Copy link
Contributor Author

0xNeshi commented Aug 9, 2023

@stevieraykatz found an issue with previous implementation where it wouldn't preserve gas percent split between locked and liquid on strategyRedeem (previously was split proportional to amount to redeem from locked/liquid, but now split in half by default), so updated the code in a way that preserves this behavior while addressing the issue at hand, see commit messages and new description.

@0xNeshi
Copy link
Contributor Author

0xNeshi commented Aug 9, 2023

Closing, will be part of PR #267 which will include additional tests for the changes

@0xNeshi 0xNeshi closed this Aug 9, 2023
@0xNeshi 0xNeshi deleted the acc-strat-fixes branch August 9, 2023 07:38
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants