-
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: rfox refactor #7079
feat: rfox refactor #7079
Conversation
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.
So we dont let the resolutions in #7020 regarding non-null assertions, marking this as requested changes until we address that feedback as its relevent here too.
#7020 (review)
36ff866
to
d221e62
Compare
0e6df8c
to
c32b72b
Compare
96f010a
to
62532f1
Compare
cddf7cb
to
b16682b
Compare
b950ec5
to
ff065af
Compare
src/lib/swapper/swappers/ArbitrumBridgeSwapper/getTradeQuote/getTradeQuote.ts
Show resolved
Hide resolved
src/lib/swapper/swappers/ArbitrumBridgeSwapper/getTradeQuote/getTradeQuote.ts
Show resolved
Hide resolved
49d4a3f
to
dbc5531
Compare
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.
Absolutely tasty PR ser, just minor non-blocking feedback regarding memoization for perf and some non-blocking stylistic querstions/comments.
Type errors after develop merge fixed in #7094 |
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.
I didn't test it yet but love the code for now! Well done
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.
Staking preview
Lock up warning
Confirm
Confirm and stake, loading and success states
Looks super sane, appreciate the effort on taking care of the status like I did on LP deposits
Unstake 50%
Everything looks smooth on my side
Very nitpick but this shouldn't be a button when hovering but an underlined link without the background color
And the claim loader could be a bit sexier, to be tackled in an UI refining PR I guess
Claim list
Cooldown period looks fine, available claim too
🚫 Claim
I'm not able to claim at all
Change address
Both custom address and put current wallet address back are happy
LGTM except for the claim part which doesn't look happy 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.
Description
a.k.a originally RFOX bridge refactor to hide logic away into hooks, effectively the same for stake and unstake, react-query type-safety, and much more, see below.
useRfoxBridge
anduseRfoxApproval
queries to keep things cleanbalanceOf
andstakingInfo
) into reusable hooks, leveraging@wagmi/core
's internal to get theirskipToken
type-safe variant and keeping things DRYskipToken
type safety (see ff065af)query-key-factory
to latestPull Request Type
Issue (if applicable)
other RFOX bits(disregard this, gone rambo with the refactor it and did it errwhere in RFOX) and other parts of the codebase to minimize/nuke the crazy deps/hooks boilerplate in data-fetching queries.skipToken
type-safety #6973EvmFees
typeRisk
Low - moved code, and retested E2E - ensure RFOX as a feature, with particular focus on RFOX bridge still works as it did in the PR under
Testing
Engineering
Operations
Screenshots (if applicable)