fix: update swap lifecyclestatus #1249
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
alessey
force-pushed
the
alessey/lifecyclestatusV2
branch
from
f4f4ce4 to
5a8edd8
Compare
alessey
force-pushed
the
alessey/lifecyclestatusV2
branch
from
f310997 to
bd087b0
Compare
alessey
force-pushed
the
alessey/lifecyclestatusV2
branch
from
bd087b0 to
138f24a
Compare
alessey
force-pushed
the
alessey/lifecyclestatusV2
branch
from
138f24a to
f75d96f
Compare
alessey
force-pushed
the
alessey/lifecyclestatusV2
branch
from
f75d96f to
4c096e9
Compare
alessey
force-pushed
the
alessey/lifecyclestatusV2
branch
from
4c096e9 to
d9221b0
Compare
| }); | ||
| }, | ||
| [], | ||
| ); |
There was a problem hiding this comment.
This is the main update, creating a setState callback function here that persists all statusData except errors, this data can be overwritten, but will persist until init is called again. This has a slightly different type than LifeCycleStatus since shared data is persisted it is not required except in init.
There was a problem hiding this comment.
very very clever, and can't wait for next PRs to have this abstract even more, so we can use it in other components.
| !from.token || !to.token || !from.amount || !to.amount, | ||
| }, | ||
| }); | ||
| }, [from, to, updateLifeCycleStatus]); |
There was a problem hiding this comment.
It seemed like we should be triggering an amountChange on toggle since from and to switch, thoughts?
| isMissingRequiredField: false, | ||
| maxSlippage, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
I don't believe this is needed, unless it is possible to swap even if the quote fails. Status init doesn't make sense at this point when you are in the middle of the process.
| >; | ||
| }) | ||
| : never | ||
| : never; |
There was a problem hiding this comment.
this is some complex ts to make LifeCycleStatusDataShared optional when updating state in all statuses except for init. the alternative would just be to explicitly duplicate LifeCycleStatus with this logic, but would require adding/updating state types in two places.
There was a problem hiding this comment.
As this complexity will be shared across many experiences, this is a complexity that is fair to pay.
Plus, for everyone reading, we tested all other options and we still found this trade-off being the most simple overall.
| transactionHash: txHash, | ||
| transactionType: useAggregator ? 'ERC20' : 'Permit2', | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Currently we are showing confirm in wallet until the receipt, seemed like we should emit the transaction status and then you'll get success after receipt.
There was a problem hiding this comment.
I lean towards your point, but can we ask also @0xAlec @cpcramer @mykcryptodev @abcrane123 what they think about this.
| } | ||
| if (lifeCycleStatus.statusName === 'transactionApproved') { | ||
| setPendingTransaction(false); | ||
| } |
There was a problem hiding this comment.
no more reacting to state changes to set state
| @@ -151,7 +168,6 @@ export function SwapProvider({ | |||
| dToken?: Token, | |||
| // biome-ignore lint/complexity/noExcessiveCognitiveComplexity: TODO Refactor this component | |||
| ) => { | |||
| const maxSlippage = lifeCycleStatus.statusData.maxSlippage; | |||
| }; | ||
|
|
||
| // make all keys in T optional if they are in K |
| @@ -32,23 +29,25 @@ export function getSwapMessage({ | |||
| return SwapMessage.INSUFFICIENT_BALANCE; | |||
| } | |||
| // handle pending transaction | |||
| if (isTransactionPending) { | |||
| if (lifeCycleStatus.statusName === 'transactionPending') { | |||
|
Great job @alessey !!! |
What changed? Why?
Notes to reviewers
How has it been tested?