-
Notifications
You must be signed in to change notification settings - Fork 191
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
Conversation
package.json
Outdated
@@ -207,6 +207,7 @@ | |||
"uuid": "^9.0.0", | |||
"vaul": "^0.9.0", | |||
"viem": "^1.16.6", | |||
"wagmi": "1.4.13", |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 🎉
@@ -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>> = { |
There was a problem hiding this comment.
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
There was a problem hiding this 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:
❓ I'm not sure I understand this, is it 1 hour or 28 days?
❓ 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
packages/chain-adapters/src/cosmossdk/thorchain/ThorchainChainAdapter.ts
Show resolved
Hide resolved
…ncy when `isFiat`
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. https://jam.dev/c/c57272f9-9ddb-4c33-bac3-8d50ceebd751
There are two things off here:
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? Fixed in bbacc30:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return false | ||
}, [isApprovalRequired, isApprovalTxSuccess]) | ||
|
||
console.log({ isApprovalRequired, isTransitioning }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rm
Nice catch @0xApotheosis - tackled in #6989 as a follow-up! |
Description
This PR implements RFOX staking e2e.
validateAddress
in THOR adapter<TradeAmountInput />
has been touched in a non-breaking manner to now consume a form context if present withuseFormContext
or have a dummyuseFormContext
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 theregeneratedAssetData
patching for missing FOX on arb assetPull Request Type
Issue (if applicable)
closes #6737
Risk
Low/medium
Testing
Engineering
Operations
Screenshots (if applicable)
https://jam.dev/c/ef9c0848-d978-459e-b209-5b57c4b1257f