-
Notifications
You must be signed in to change notification settings - Fork 635
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
fix position total value display #6185
fix position total value display #6185
Conversation
src/resources/defi/utils.ts
Outdated
* omittedTotal is the total WITHOUT the positions that are omitted | ||
* used for the total wallet balance, the position card still shows the normal total | ||
*/ | ||
omittedTotal: { |
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.
found this name (ommitedTotal
) kinda confusing but couldn't think of anything better, lmk if you think of a more descriptive name
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.
idk maybe totalWithOmissions
?
also ommited is spelled omitted btw
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 think it would be a lot more clear if we first came up for a name for the types of positions that are being excluded. i'm still unclear on this. what dequalifies a position from having it's value being included in the total wallet balance?
src/resources/defi/utils.ts
Outdated
totalBorrows = add(totalBorrows, nativeDisplay.amount); | ||
|
||
if (borrow.omit_from_total) { | ||
totalOmittedFromTotals = subtract(totalOmittedFromTotals, nativeDisplay.amount); |
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 make sense? we add the others but subtract the borrows
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 mean it works right, not sure if I missed some edge case
@greg-schrammel, code looks good but I'm unsure what to test. Is the moxie case the only thing I should test or are there other considerations? |
yeah the only positions I know that can have the |
src/resources/defi/utils.ts
Outdated
* omittedTotal is the total WITHOUT the positions that are omitted | ||
* used for the total wallet balance, the position card still shows the normal total | ||
*/ | ||
omittedTotal: { |
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.
idk maybe totalWithOmissions
?
also ommited is spelled omitted btw
src/resources/defi/utils.ts
Outdated
* omittedTotal is the total WITHOUT the positions that are omitted | ||
* used for the total wallet balance, the position card still shows the normal total | ||
*/ | ||
omittedTotal: { |
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 think it would be a lot more clear if we first came up for a name for the types of positions that are being excluded. i'm still unclear on this. what dequalifies a position from having it's value being included in the total wallet balance?
Fixes APP-####
What changed (plus any additional context for devs)
some positions are omitted from the total wallet balance, but should still display the position value in the position card
Screen recordings / screenshots
before/after
What to test