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

feat: wire-up rfox staking #6925

Merged
merged 94 commits into from
May 24, 2024
Merged

feat: wire-up rfox staking #6925

merged 94 commits into from
May 24, 2024

Conversation

gomesalexandre
Copy link
Contributor

@gomesalexandre gomesalexandre commented May 15, 2024

Description

This PR implements RFOX staking e2e.

  • Wallet/custom RUNE address with newly added bech32 validateAddress in THOR adapter
  • balance and fee asset balance validation, using react-hook-form. <TradeAmountInput /> has been touched in a non-breaking manner to now consume a form context if present with useFormContext or have a dummy useFormContext otherwise. We actually declare the form context up top in RFOX's <StakeInput /> and do anything with said consumed form context in <TradeAmountInput /> meaning this isn't a user-facing change despite the seemingly scary diff there
  • Handles approval needed/allowance Tx firing
  • Handles signing and broadcasting the Tx, including EIP1559 support
  • Displays approval/estimate fees programmatically
  • Displays share of pool
  • Handles loading states gracefully
  • Adds some market-data patching for missing FOX on Arb market data
  • Adds some generatedAssetData patching for missing FOX on arb asset
  • Adds FOX on arb and RFOX contract address + RFOX ABI constants (to be updated with latest once @0xean's feat: refactor for staking token to be generic rFOX#43 goes in and v2 is deployed)
  • Reintroduces wagmi for QoL (no need to reinvent the wheel when reading data with our own react-query hooks when battle-tested ones exist)
  • Bumps viem from v1 -> v2 to work nicely with wagmi v2, and tackles all type errors and breaking changes (i.e function and arg name changes)
TODO:
- [x] tackle comments in this guy https://github.com/shapeshift/web/blob/7f85132b0f6b73cd77d8078ec060ebe5ae17da63/src/components/MultiHopTrade/components/TradeAmountInput.tsx
- [x] regression test added `useFormContext` and related changes in `<TradeAmountInput />` don't cause in regressions in current consumers
- [x] market patch FOX on Arb market data
- [x] approval fee instead of gas fee at input time when required
- [x] find a solution for `generatedAssetData.json` madness
- [x] share of pool?
- [x] Scrutinize react-queries that live in a wrong place currently (i.e lending)/should be generalized here, this is a bit messy on the query side and not really leveraging our current react-queries reusable queries for some
  - [x] 'estimateFees' (and consume the abstracted version in lending too after ensuring the implementation is line-per-line the exact same)
  - [x]  yeah no, we need the current mutation for proper loading state after click ~~send Tx mutation - probably not a now thing though it looks like we don't need a mutation at all here since we're not doing anything complex that requires a mutation - just move the logic to `handleStake` itself~~
- [x] Contract ABI and consumption
- [x] cooldownPeriod query
- [x] input
  - [x] estimate gas when approval *not* needed
  - [x] balance checks
  - [x] balance + fees checks as additional validate method
- [x] confirm
  - [x] approval step when required
  - [x] Tx broadcast
- [x] status
- [x] Record a final commented jam before opening this commenting on every row and ensuring data is there and things are happy re: balance checks, approval, spamming the delete key and re-inputting ensuring no race conditions etc. Go with the full flow e2e, including custom receive addy and its RUNE bech32 validation

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)

closes #6737

Risk

High Risk PRs Require 2 approvals

Low/medium

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

Testing

  • RFOX flow is happy e2e, including
    • Approval needed (approval step is triggered and approval Tx is happy)
    • Not enough fee for gas
    • Not enough FOX balance for entered amount
    • Manual RUNE address (try with an invalid address too and ensure you cannot continue), as well as account RUNE addies
  • Do a quick regression test of inputs in THOR LP, swapper and lending. No need to test these end-to-end, just ensure that you can still type amounts there without inputs being rugged

Engineering

  • ☝🏽

Operations

  • ☝🏽

Screenshots (if applicable)

https://jam.dev/c/ef9c0848-d978-459e-b209-5b57c4b1257f

@gomesalexandre gomesalexandre requested a review from a team as a code owner May 15, 2024 21:55
@gomesalexandre gomesalexandre changed the title staking components [skip ci] wip: wire-up rfox staking May 15, 2024
package.json Outdated
@@ -207,6 +207,7 @@
"uuid": "^9.0.0",
"vaul": "^0.9.0",
"viem": "^1.16.6",
"wagmi": "1.4.13",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah we're bringing this back, no reason to reinvent the wheel when reading data in components - wagmi is just a wrapper around react-query and we would write ours, not so well optimized anyway, using viem.

Since wagmi wraps react-query and uses our viem client under the hood, that's literally the same we would write, but better. Nothing crazy huge either: https://bundlephobia.com/package/[email protected]

Note: this is the last v1 before wagmi published v2 which has viem v2 as a dependency, meaning we'd also have to bump viem to v2. Initially went with that and this is nothing super bad to tackle, but this PR is probably not the place for it so staying at v1 for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually bumped viem and wagmi to latest, tackled all type errors and renames following breaking change. Now bundle is actually twice smaller 🎉

@gomesalexandre gomesalexandre marked this pull request as draft May 15, 2024 23:14
src/lib/utils/time.ts Outdated Show resolved Hide resolved
@@ -56,7 +56,7 @@ export const viemPolygonClient = createPublicClient({
transport: http(getConfig().REACT_APP_POLYGON_NODE_URL),
})

export const viemClientByChainId: Record<EvmChainId, PublicClient> = {
export const viemClientByChainId: Record<EvmChainId, PublicClient<Transport, Chain>> = {
Copy link
Contributor Author

@gomesalexandre gomesalexandre May 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was not typed properly but just happened to work given our usage, doesn't anymore

@0xApotheosis 0xApotheosis self-assigned this May 23, 2024
Copy link
Contributor

@0xApotheosis 0xApotheosis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Holy moly this is a monster PR, well done ser.

Code review pass and light functional test:

✅ Can successfully stake FOX on Arbitrum

TradeAmountInput consumers are still happy :fridaydog:

❌ Fiat amounts are sad, but only when connected to an account with no FOX on Arbitrum:

rFOX-fiat-sad.mp4

❌ Share of pool looks off in both the quot and the confirm screens:

Screenshot 2024-05-23 at 3 07 00 PM Screenshot 2024-05-23 at 3 12 23 PM

❓ I'm not sure I understand this, is it 1 hour or 28 days?

Screenshot 2024-05-23 at 3 07 39 PM

❓ Approval does not show a transaction ID, and is a little jumpy (could use some loading states on the button and on the gas estimate):

rFOX-approval.mp4

src/lib/utils/time.ts Outdated Show resolved Hide resolved
src/lib/wagmi-config.ts Show resolved Hide resolved
src/pages/RFOX/components/AddressSelection.tsx Outdated Show resolved Hide resolved
@gomesalexandre
Copy link
Contributor Author

❌ Fiat amounts are sad, but only when connected to an account with no FOX on Arbitrum:

rFOX-fiat-sad.mp4

Nice find. Looking closely, we can see that the opposite fiat value is also missing when inputting crypto. The issue here is we needed to hydrate FOX on Arbitrum market data. Mainnet FOX market-data would conventionally be hydrated across the app regardless of whether or not the user has balance (many many consumers) and isn't an issue, but FOX on Arbitrum isn't.
Fixed in b5b8d3d - tangential, but also passed the cryptoAmount in to <TradeAssetInput /> too for sanity in cdf3dfe, and while at it, added missing prefix for crypto as opposite currency when isFiat in <TradeAmountInput />

https://jam.dev/c/c57272f9-9ddb-4c33-bac3-8d50ceebd751

❌ Share of pool looks off in both the quot and the confirm screens:

Screenshot 2024-05-23 at 3 07 00 PM Screenshot 2024-05-23 at 3 12 23 PM

There are two things off here:

  1. the whole share of pool thing isn't very clear and
  2. derp, this is an actual bug, forgot to cherry-pick math fixes from the unstaking PR, consuming stakingInfo[2], i.e the staking balance, vs. the wrong hollistic balanceOf, which includes pending unstaking too.

as for 1. tl;dr, we've been back and forth with this one with beard many times, and this isn't super clear either for engineering nor users. Is it your current share of pool? Is it the share of pool for the exact amount you're staking, but not taking into account your currently staked amount? Is it the new share of pool after you stake said amount?
The answer is the latter, but that's not super obvious at all UX-wise. We've agreed to do a follow-up to use the same <DynamicComponent /> as lending, which will show both before and after values in the spirit of clarity.

Fixed in bbacc30:

  • Current contract balance: 123 FOX

image

  • Current user staking balance: 10.6875 FOX i.e (10.6875 / 123) = 8.68902439 % of the pool

image

  • Staking 0 - no change of pool share

image

  • Staking a fraction of FOX - no visible change of pool share given the toFixed(2) and rounding for percentage, but allows us to confirm confirm is happy

image

image

  • Staking 100 FOX - i.e new staking balance = 110.6875. New share of pool = (110.6875 / (123 + 100)) = 49.635650224 %

image

❓ I'm not sure I understand this, is it 1 hour or 28 days?

8196a79

image

Screenshot 2024-05-23 at 3 07 39 PM

❓ Approval does not show a transaction ID, and is a little jumpy (could use some loading states on the button and on the gas estimate):

rFOX-approval.mp4

Copy link
Contributor

@0xApotheosis 0xApotheosis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still a few small things to do as follow-ups, which I'll outline below, but let's get this monster in 🙏

  • Approval gas estimate changes after confirming quote:
changing-approval-gas.mp4
  • Approval fee fiat amount spans two lines:
Screenshot 2024-05-24 at 9 37 34 AM

return false
}, [isApprovalRequired, isApprovalTxSuccess])

console.log({ isApprovalRequired, isTransitioning })
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rm

@0xApotheosis 0xApotheosis merged commit e396ffb into develop May 24, 2024
6 checks passed
@0xApotheosis 0xApotheosis deleted the feat_wire_up_rfox_staking_1 branch May 24, 2024 01:20
@gomesalexandre
Copy link
Contributor Author

Still a few small things to do as follow-ups, which I'll outline below, but let's get this monster in 🙏

  • Approval gas estimate changes after confirming quote:

changing-approval-gas.mp4

  • Approval fee fiat amount spans two lines:
Screenshot 2024-05-24 at 9 37 34 AM

Nice catch @0xApotheosis - tackled in #6989 as a follow-up!

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.

Implement rFOX staking flow
5 participants