Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Loans: linear accrual on settlement price #1739
Loans: linear accrual on settlement price #1739
Changes from 9 commits
31ee696
f61896f
b533b75
19a085f
d8d4a9a
256f726
ff13d34
39d2e3f
e52e958
0dd088d
574eeed
0456a3d
9ba97d8
87dfe1b
b760ad6
bd5c8f6
fd3dab6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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 we could use the valuation method of the internal pricing here too. Removing the Option. ANd I would name it current_value.
cc: @denniswell
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.
Valuation method is just the present value for internal pricing loans. So we should do the same for external pricing loans too if go with this change.
This is related to my question:
For external pricing loans
present_value = quantity * latest_settlemente_price
. So maybe we do not need to use another field here and just update howpresent_value
is computed using nowcurrent_price
instead oflatest_settlement_price
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.
Ah sorry, my bad! I think that is the best idea!
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.
Maybe we still want to put
current_price
here to allow the UI just to draw the price. Otherwise, they should divide the present_value / outstanding_quantity to obtain it.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 that is confusing more ATM. The total value is anyways of greatest interest.
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 all that is relevant mostly is the value anyways.
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.
Regarding this topic, I would say that if Apps needs to draw the price, leave the
current_price
as it is. Otherwise, if they need it and we do not offer it by the RuntimeAPI, then they need to perform not-trivial computations, and future changes to present_value will break that.@onnovisser are you drawing right now the
settlementPrice
of an external loan?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.
@lemunozm can we remove that one here?
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 we decide after getting feedback from https://kflabs.slack.com/archives/C04GARZ532T/p1709024010720729 ?
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.
Onno is out, which is who is taking care of it, I think, so probably we do not have feedback soon regarding this.
Seeing the apps code, they need to fetch the price as
current_price
now to fulfill this part: https://github.com/centrifuge/apps/blob/beea676a936a14597f270e931387a4df1ce7ca0d/centrifuge-app/src/utils/getLatestPrice.ts#L10 which is later used to draw in the loan page.I would say we need to keep this here. Do you have any argument against this, @mustermeiszer? I do not see you very in favor of adding it.