-
Notifications
You must be signed in to change notification settings - Fork 42
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.
All I want for Christmas 🎵
One minor thing re: FRT's, else the rest is looking good.
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.
A few nits. This is looking reasonable so far, but no ready to approve until I can get this table another look
|
||
Bonding is described in more detail in | ||
<<{root-prefix}bonding/index#bonding,its own section>>. | ||
|
||
==== Distributed key generation | ||
|
||
:threshold-signature: footnote:[Threshold signatures allow a group of N \ |
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.
Might deserve a link to the signing section
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.
We'll need to rewrite it, but then that's probably the right move.
docs/redemption/index.adoc
Outdated
[#deposit-payment-flow,%header,cols="1,1,1,1,1,1a"] | ||
.Deposit payment flow | ||
|=== | ||
| Deposit state | TDT holder | FRT holder | Redeemer | Repayment Amount | Disbursal Amounts |
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 there's a better way to show this. It's difficult to review- leaving this comment here for myself to come back to
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 probably add a diagram, but the table is valuable IMO. It basically summarizes the entire redemption section… And would be a great target for generating automated spec tests.
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.
Within the context of the table, a couple of other approaches I considered but hit pause on so I could get something out the door:
- Separating pre- and at-term redemption into distinct tables.
- Listing out the parties in separate columns.
Some more ideas that could be helpful:
- Doing two sub-rows in the TDT/FRT/Redeemer columns indicating disbursal amounts. That is, the second, third, and fourth column would have the party in the top half and how much they receive during redemption in the second half.
- Using something different for A, B, and C that would make them more visually distinctive, so it's easier to map between the 2/3/4 columns and the disbursal listings.
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.
Moving the amounts to the top rather than eg ref'ing "50 basis points" everywhere, replacing "BTC lot" with an amount; and replacing any math with the final amount and the math as a parenthetical would go a long way to making this digestible
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.
Did a pass at this in the new appendix: http://docs.keep.network/tbtc/spec-updates/index.html#_redemption_payment_and_disbursal_scenarios.
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.
Just a quick one, will give it a deeper look soon
Pushed a commit that addresses most of the notes. |
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.
A couple small comments, but 👍
It'd also be nice to see the governable bonus to FRT holders in the spec, but that might be outside the PR
docs/redemption/index.adoc
Outdated
[#deposit-payment-flow,%header,cols="1,1,1,1,1,1a"] | ||
.Deposit payment flow | ||
|=== | ||
| Deposit state | TDT holder | FRT holder | Redeemer | Repayment Amount | Disbursal Amounts |
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.
Moving the amounts to the top rather than eg ref'ing "50 basis points" everywhere, replacing "BTC lot" with an amount; and replacing any math with the final amount and the math as a parenthetical would go a long way to making this digestible
Emdashes and semicolons as relevant.
In a few places, it pays to clarify specific details that apply to tBTC v1, and how they could change in future versions.
Some had no anchors and now do, while others had labels that were half-words partialized with no particular rhyme or reason, and are now full words or terms for clarity.
None of these add materially new information, but they do provide some targeted clarification in places where interconnections between various bits of the mechanism are less clear.
Since the spec now calls for using Maker's medianizer, updating the feed is something that is handled external to the system.
This includes a new table that clearly outlines repayment amounts in various scenarios. It also includes a clear articulation of pre- and at-term redemption flows, and the meaning of "term".
Thank everything for spellcheck!
The appendix also provides some additional context.
Constants are now uniformly referenced from constants.adoc, so they can be managed and updated from a central location. In several cases, the constant names have also been clarified to reflect the fact that they are global across the spec.
Clarify the amounts by doing the math ahead of time rather than writing it out in line in the table.
This is the at-term scenario where the FRT holder and redeemer are the same, but the TDT holder is different.
Rational signers will maintain honest behavior, but signers in practice may not be rational. Still, they should maintain honest behavior.
Extra slashes and missing /indexes begone!
v1 will not allow for topping up collateral. Also, tweak pre-liquidation link anchor while we're in there.
Plus some wrapping tweaks.
3db81a5
to
9235765
Compare
Okayyyy I've cleaned up commit history, updated the PR description, and marked this ready for merge. |
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.
🎉🎉🎉
Excited to make this official.
This pulls together several pending spec updates. In particular:
constants.adoc
file.The spec has also gained a new appendix that describes the repayment amounts in various TDT/FRT/redeemer combinations for pre- and at-term deposits.
See #293.
Closes #301.