-
Notifications
You must be signed in to change notification settings - Fork 44
feat: loan management form add collateral #1715
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
base: main
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
apps/main/src/llamalend/features/manage-loan/components/AddCollateralForm.tsx
Outdated
Show resolved
Hide resolved
apps/main/src/llamalend/widgets/manage-loan/LoanInfoAccordion.tsx
Outdated
Show resolved
Hide resolved
apps/main/src/llamalend/widgets/manage-loan/LoanFormTokenInput.tsx
Outdated
Show resolved
Hide resolved
apps/main/src/llamalend/widgets/manage-loan/LoanInfoAccordion.tsx
Outdated
Show resolved
Hide resolved
apps/main/src/llamalend/features/manage-loan/components/AddCollateralForm.tsx
Outdated
Show resolved
Hide resolved
apps/main/src/llamalend/queries/add-collateral/add-collateral-gas-estimate.query.ts
Outdated
Show resolved
Hide resolved
| import { userMarketValidationSuite } from '@ui-kit/lib/model/query/user-market-validation' | ||
| import type { Decimal } from '@ui-kit/utils' | ||
|
|
||
| export type UserState = { |
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.
| export type UserState = { | |
| type QueryData<TUseQuery extends (...args: any[]) => any> = NonNullable<ReturnType<TUseQuery>['data']> | |
| export type UserState = QueryData<typeof useUserState> |
Personally I'd rather deduce types implicitly rather than write types as bookkeeping, prevents having to define things twice or more.
We can probably put the QueryData type in a shared places, perhaps in curve-ui-kit\src\lib\queries\types.ts.
Extracting and exporting whatever's returned by queries can be be defined simply as export type UserState = QueryData<typeof useUserState>
Don't forget to remove the Promise<UserState> return type from the queryFn.
Thoughts @DanielSchiavini?
(not a blocker btw, just my personal opinion)
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.
Sounds cleaner and more maintanable to me
apps/main/src/llamalend/widgets/manage-loan/LoanInfoAccordion.tsx
Outdated
Show resolved
Hide resolved
apps/main/src/llamalend/widgets/manage-loan/LoanInfoAccordion.tsx
Outdated
Show resolved
Hide resolved
apps/main/src/llamalend/widgets/manage-loan/LoanInfoAccordion.tsx
Outdated
Show resolved
Hide resolved
apps/main/src/llamalend/features/manage-loan/components/AddCollateralForm.tsx
Show resolved
Hide resolved
feat: loan management form remove collateral
|
|
||
| const errorMessage = (typeof error === 'object' && error?.message) || (typeof error === 'string' && error) | ||
| const showPrevValue = value != null && prevValue != null | ||
| const showPrevValue = isSet(value) && isSet(prevValue) |
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.
Wouldn't const showPrevValue = !!value && !!prevValue have sufficed? 😄
Or something along those lines
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.
Well if value or prevValue is 0, you still want to treat it as a truthy value. That said, you could argue that we don’t use numbers anymore, only Decimal. This approach is more robust imo
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 is incorrect though, cause if value and prevValue are ReactNode, then values like false should also be hidden
|
Should we abbreviate the debt and collateral numbers with 'k' and such as we do in a lot of places? These big numbers look a bit ugly. But you could argue that you want the 'exact' data 🤔 Should we also get rid of the 'max removable' helper message? It offers the exact same functionality now as with the old |
Regarding the collateral, the point was to show the exact data. If I add a small amount of BTC to my position, it could get lost due to rounding, which would make the accordion information not very useful. Yes, that’s definitely something I wanted to address as well. We currently have three different ways to select the max removable amount: the helper message, the position balance, and the MAX button. We should probably remove one or two of these, perhaps the helper message and the MAX button? |
| const value = | ||
| values.userCollateral != null && | ||
| state?.collateral != null && | ||
| decimal(new BigNumber(values.userCollateral).plus(state.collateral).toString()) | ||
| return value ? { value, tokenSymbol: collateralToken?.symbol } : null |
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.
Can't we just
| const value = | |
| values.userCollateral != null && | |
| state?.collateral != null && | |
| decimal(new BigNumber(values.userCollateral).plus(state.collateral).toString()) | |
| return value ? { value, tokenSymbol: collateralToken?.symbol } : null | |
| values.userCollateral && state?.collateral && { | |
| value: decimal(new BigNumber(values.userCollateral).plus(state.collateral)), | |
| tokenSymbol: collateralToken?.symbol | |
| } |
Note that decimal should support BigNumber imo
| state?.collateral != null && | ||
| values.userCollateral != null && | ||
| decimal( | ||
| BigNumber.max(new BigNumber(state.collateral).minus(new BigNumber(values.userCollateral)), '0').toString(), |
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.
| BigNumber.max(new BigNumber(state.collateral).minus(new BigNumber(values.userCollateral)), '0').toString(), | |
| BigNumber.max(0, new BigNumber(state.collateral).minus(new BigNumber(values.userCollateral))), |
| () => ({ balance, symbol: token?.symbol, loading: isBalanceLoading }), | ||
| [balance, isBalanceLoading, token?.symbol], | ||
| )} | ||
| walletBalance={walletBalance} |
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.
Argh this is not a real wallet balance anymore, and tbh LargeTokenInput should not care.
As discussed, for future developments let's try to pass generic ReactNodes
| balance: position ? position.data : balance, | ||
| symbol: token?.symbol, | ||
| loading: position ? position.isLoading : isBalanceLoading, |
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.
| balance: position ? position.data : balance, | |
| symbol: token?.symbol, | |
| loading: position ? position.isLoading : isBalanceLoading, | |
| balance: position?.data ?? balance, | |
| symbol: token?.symbol, | |
| loading: position?.isLoading ?? isBalanceLoading, |
| tokenAddress: token?.address, | ||
| }) | ||
|
|
||
| const position = positionBalance?.position |
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.
| const position = positionBalance?.position | |
| const { position, tooltip } = positionBalance ?? {} |
|
|
||
| const errorMessage = (typeof error === 'object' && error?.message) || (typeof error === 'string' && error) | ||
| const showPrevValue = value != null && prevValue != null | ||
| const showPrevValue = isSet(value) && isSet(prevValue) |
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 is incorrect though, cause if value and prevValue are ReactNode, then values like false should also be hidden
| large: 'headingSBold', | ||
| } as const satisfies Record<ActionInfoSize, TypographyVariantKey> | ||
|
|
||
| const isSet = (v: ReactNode) => v != null && v !== '' |
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.
As mentioned below, ReactNodes can be false too
| const isSet = (v: ReactNode) => v != null && v !== '' | |
| const isSet = (v: ReactNode) => v || v === 0 |
| */ | ||
| tokenSelector?: ReactNode | ||
|
|
||
| // TODO: rename to just "balance" because multipurpose now (walletBalance, positionBalance ...) |
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 there is already a balance prop
| // TODO: rename to just "balance" because multipurpose now (walletBalance, positionBalance ...) | |
| // TODO: receive a `maxBalance` ReactNode to allow anything to be injected |


collateral,prevCollateral,prevHealth,prevDebtandprevLTVto the accordiongetErrorMessagefrom/loanand/lendint/llamalendGracefully handle user rejection (unlike my Ex):