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

feat: savers remove dangerous withdraw warning #7107

Merged

0xApotheosis merged 8 commits into develop from feat_savers_rm_dangerous_withdraw

Jun 17, 2024

Contributor

gomesalexandre commented Jun 12, 2024 •

edited

Loading

... and a few more things.

Description

See #6707 (comment)

finally managed to repro withdraws on BTC despite current fees endpoint shenanigans and it looks like indeed, we can remove these, since the minimum effectively catches dangerous withdraws, meaning we don't need neither the current dangerousWithdrawWarning nor the new ack?

Precisely what the comment says - we can't get into a dangerous withdraw state meaning this is effectively dead code.

Whilst in the house:

fixes precision issues (see inline comments)
makes the withdraw query type-safe
does proper staking balance checks so we don't run the query and end up with bps > 10_000, resulting in THOR erroring on us, and "Amount too low" (how can amount be too low if I'm inputing more than my staked balance?)
does general cleanup in the withdraw input component

Pull Request Type

🐛 Bug fix (Non-breaking Change: Fixes an issue)
🛠️ Chore (Non-breaking Change: Doc updates, pkg upgrades, typos, etc..)
💅 New Feature (Breaking/Non-breaking Change)

Issue (if applicable)

contributes to Complete WarningAcknowledgement implementation #6707

Risk

High Risk PRs Require 2 approvals

Low but if I got this wrong and we can get in this state, then problems.

What protocols, transaction types or contract interactions might be affected by this PR?

Testing

Dangerous withdraws

Confirm I'm not derp and we indeed can't get into a dangerous withdraw state. How to do that, you ask?

With the network tab open and filtered by close, filter requests by withdraw and click withdraw on a savers position
Input an amount low enough for THOR quote endpoint to respond with expected_amount_out: '' (meaning fees eat the withdraw and nothing is effectively withdrawn)
Confirm for said cases of expected_amount_out: '' (i.e guaranteed burned fundus), we can't continue past input and this is guarded against with "Minimum: "
Now that you know the minimum (keep in mind that this amount is constantly changing, so don't wait too much or the minimum may have changed since we queried it), try and input it
Ensure you can now proceed, and expected_amount_out is non-empty in the quote response

Minimum amounts

Entering the minimum amount on a saver withdraw position should now be happy

Amout gt position

Typing an amount greater than your staking position balance should now fail at "Insufficient Funds" - instantly, without a network request

Precision shenanigans

Percent option click should input an amount in the correct precision (e.g 8 for BTC or 6 for ATOM)

Engineering

^

Operations

^

Screenshots (if applicable)

USDC on AVAX

Screenshot 2024-06-12 at 17 16 21

Screenshot 2024-06-12 at 17 16 40

Screenshot 2024-06-12 at 17 16 58

Screenshot 2024-06-12 at 17 16 58

Screenshot 2024-06-12 at 17 16 58

Screenshot 2024-06-12 at 17 16 58

BTC

Screenshot 2024-06-12 at 19 07 10

Screenshot 2024-06-12 at 19 07 23

Screenshot 2024-06-12 at 19 07 40

Screenshot 2024-06-12 at 19 07 50

Screenshot 2024-06-12 at 19 09 06

AVAX

Screenshot 2024-06-12 at 17 29 10

Screenshot 2024-06-12 at 17 32 47

Screenshot 2024-06-12 at 17 32 47

Screenshot 2024-06-12 at 17 35 49

Screenshot 2024-06-12 at 17 36 03

Cosmos

Cosmos send endpoint seemingly borked on sends so couldn't go to completion on this one but:


          feat: savers remove dangerous withdraw warning

3ef2781

gomesalexandre requested a review from a team as a code owner

June 12, 2024 15:26

gomesalexandre marked this pull request as draft

June 12, 2024 15:26

gomesalexandre added 2 commits

June 12, 2024 17:50


          fix: precision things

c1acdea


          fix: rugged bps when input gt staked balance

0b9ec69

gomesalexandre force-pushed the feat_savers_rm_dangerous_withdraw branch from 2d8cd1c to 0b9ec69 Compare

June 12, 2024 17:10

gomesalexandre commented

View reviewed changes

...roviders/thorchain-savers/components/ThorchainSaversManager/Withdraw/components/Withdraw.tsx

               import { BASE_BPS_POINTS } from 'lib/utils/thorchain/constants'
               import type { GetThorchainSaversWithdrawQuoteQueryKey } from 'lib/utils/thorchain/hooks/useGetThorchainSaversWithdrawQuoteQuery'
               import {
-                queryFn as getThorchainSaversWithdrawQuoteQueryFn,
+                fetchThorchainWithdrawQuote,

Contributor Author

gomesalexandre Jun 12, 2024

I.e just a good ol' async fn now, no need to tie it to react-query terminology

gomesalexandre commented

View reviewed changes

...roviders/thorchain-savers/components/ThorchainSaversManager/Withdraw/components/Withdraw.tsx

Comment on lines +75 to +80

+                const { control, setValue } = methods
+                const cryptoAmount = useWatch<WithdrawValues, Field.CryptoAmount>({
+                  control,
+                  name: Field.CryptoAmount,
+                })

Contributor Author

gomesalexandre Jun 12, 2024

getValues() is non-reactive and we have to opt-chain on it, v. bad

gomesalexandre commented

View reviewed changes

...roviders/thorchain-savers/components/ThorchainSaversManager/Withdraw/components/Withdraw.tsx

Comment on lines +176 to +179

+                const hasEnoughStakingBalance = useMemo(
+                  () => bnOrZero(cryptoAmount).lte(amountAvailableCryptoPrecision),
+                  [amountAvailableCryptoPrecision, cryptoAmount],
+                )

Contributor Author

gomesalexandre Jun 12, 2024

Exposed at component-level too so we can disable the query in case of no-ops.

This was the explanation for "Amount too low" when inputting more than your staked balance previously. The ratio we were doing to get the bps was resulting in > 10_000 bps, which makes sense since input/staked amount including fees is a positive ratio.

gomesalexandre commented

View reviewed changes

...roviders/thorchain-savers/components/ThorchainSaversManager/Withdraw/components/Withdraw.tsx

                     const fiatAmount = bnOrZero(cryptoAmount).times(assetMarketData.price)
                     setValue(Field.FiatAmount, fiatAmount.toString(), { shouldValidate: true })
-                    setValue(Field.CryptoAmount, cryptoAmount.toFixed(), { shouldValidate: true })
+                    setValue(Field.CryptoAmount, cryptoAmount.toFixed(asset.precision), { shouldValidate: true })

Contributor Author

gomesalexandre Jun 12, 2024

This was off, and is in fact directly related to this PR even though it doesn't look like it.

Percentage option click wasn't taking into account asset precision, and we were ending up with a value that has possibly more dp than the asset's decimals (e.g: BTC).

This is the reason why trying to input the minimum that's stated still failed validation - one amount had the right amount of decimal places, while the other didn't.

gomesalexandre commented

View reviewed changes

...roviders/thorchain-savers/components/ThorchainSaversManager/Withdraw/components/Withdraw.tsx

                 const safeOutboundFeeInAssetCryptoBaseUnit = useMemo(() => {
                   if (!outboundFeeInAssetCryptoBaseUnit) return
                   // Add 5% as as a safety factor since the dust threshold fee is not necessarily going to cut it
-                  return bnOrZero(outboundFeeInAssetCryptoBaseUnit).times(1.05).toFixed()
+                  return bnOrZero(outboundFeeInAssetCryptoBaseUnit).times(1.05).toFixed(0)

Contributor Author

gomesalexandre Jun 12, 2024

The other reason why inputing exactly the minimum amount (or just around) would still fail validation - a base unit amount should have no decimal places, and this one had some.

gomesalexandre commented

View reviewed changes

...roviders/thorchain-savers/components/ThorchainSaversManager/Withdraw/components/Withdraw.tsx

Comment on lines +333 to +334

		if (withdrawAmountCryptoPrecision.gt(amountAvailableCryptoPrecision))
		return 'common.insufficientFunds'

Contributor Author

gomesalexandre Jun 12, 2024

Don't run the query and early bail if the withdraw amount is greater than the staked balance + rewards

gomesalexandre commented

View reviewed changes

...roviders/thorchain-savers/components/ThorchainSaversManager/Withdraw/components/Withdraw.tsx

                         await queryClient
                           .fetchQuery({
                             queryKey: thorchainSaversWithdrawQuoteQueryKey,
-                            queryFn: getThorchainSaversWithdrawQuoteQueryFn,
+                            queryFn: () =>
+                              fetchThorchainWithdrawQuote({ asset, accountId, amountCryptoBaseUnit }),

Contributor Author

gomesalexandre Jun 12, 2024

Note, with the takeaways from react-query type-safety, we're not using a magic queryFn which leverages the queryKey, but a good ol' async fn which is type-safe

gomesalexandre commented

View reviewed changes

...roviders/thorchain-savers/components/ThorchainSaversManager/Withdraw/components/Withdraw.tsx

Comment on lines +477 to +478

		const hasValidStakingBalance =
		amountAvailableFiat.gt(0) && valueCryptoPrecision.gt(0) && amountAvailableFiat.gte(value)

Contributor Author

gomesalexandre Jun 12, 2024

Early bails if we don't have enough staking balance, regardless of whether this is an UTXO withdraw or not

gomesalexandre commented

View reviewed changes

...roviders/thorchain-savers/components/ThorchainSaversManager/Withdraw/components/Withdraw.tsx Outdated Show resolved Hide resolved

gomesalexandre commented

View reviewed changes

src/lib/utils/thorchain/balance.ts Outdated Show resolved Hide resolved

gomesalexandre commented

View reviewed changes

src/lib/utils/thorchain/balance.ts Outdated

Comment on lines 116 to 120

+                          getThorchainSaversWithdrawQuoteQueryFn({
+                            asset,
+                            accountId,
+                            amountCryptoBaseUnit: withdrawAmountCryptoBaseUnit,
+                          }),

Contributor Author

gomesalexandre Jun 12, 2024

Note, with the takeaways from react-query type-safety, we're not using a magic queryFn which leverages the queryKey, but a good ol' async fn which is type-safe

gomesalexandre commented

View reviewed changes

src/lib/utils/thorchain/hooks/useGetThorchainSaversWithdrawQuoteQuery.tsx Outdated

Comment on lines 33 to 38

+              export const fetchThorchainWithdrawQuote = async ({
+                asset,
+                accountId,
+                amountCryptoBaseUnit,
+                withdrawBps,
+              }: FetchThorchainWithdrawQuoteInput) => {

Contributor Author

gomesalexandre Jun 12, 2024

Note, with the takeaways from react-query type-safety, we're not using a magic queryFn which leverages the queryKey, but a good ol' async fn which is type-safe

gomesalexandre added 2 commits

June 12, 2024 19:21


          feat: self-review

0a77579


          Merge remote-tracking branch 'origin/develop' into feat_savers_rm_dan…

b7f735d

…gerous_withdraw

gomesalexandre marked this pull request as ready for review

June 12, 2024 17:27


          Merge branch 'develop' into feat_savers_rm_dangerous_withdraw

gomesalexandre mentioned this pull request

Sanitize amount inputs max digits #7125

Closed

kaladinlight reviewed

View reviewed changes

src/lib/utils/thorchain/hooks/useGetThorchainSaversWithdrawQuoteQuery.tsx Outdated

@@ @@ -63,10 +65,12 @@ export const useGetThorchainSaversWithdrawQuoteQuery = ({ @@
                 asset,
                 accountId,
                 amountCryptoBaseUnit,
+                enabled = true,
               }: {
                 asset: Asset

Contributor

kaladinlight Jun 13, 2024

Not different than previously, but feels weird to have the hook take different input args than the query fn (missing withdrawBps). Was thinking we could use the same type as well, but would have to handle undefined accountId in the query function as well

Contributor Author

gomesalexandre Jun 14, 2024

Nice catch! Initially we had support for both withdrawBps and amountCryptoBaseUnit (one or the other).
However I checked all the consumptions and we never pass in bps (not sure we ever did?), so might as well remove it.

gomesalexandre added 2 commits

June 14, 2024 08:40


          Merge remote-tracking branch 'origin/develop' into feat_savers_rm_dan…

6177e51

…gerous_withdraw


          feat: cleanup types

daa29ef

0xApotheosis self-assigned this

0xApotheosis approved these changes

View reviewed changes

Contributor

0xApotheosis left a comment

✅ Minimum amount protection is happy and safe

Screenshot 2024-06-17 at 4 00 17 PM

✅ Entering the exact minimum amount is happy

Screenshot 2024-06-17 at 4 03 28 PM

✅ Amounts greater than position show insufficient funds

Screenshot 2024-06-17 at 4 01 37 PM

✅ Precision happy, percentages work as expected

Thanks for the great testing steps!

0xApotheosis merged commit 679b850 into develop

6 checks passed

0xApotheosis deleted the feat_savers_rm_dangerous_withdraw branch

June 17, 2024 06:05

0xApotheosis mentioned this pull request

chore: release v1.617.0 #7157

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet