Skip to content
This repository has been archived by the owner on Mar 28, 2023. It is now read-only.

Initial Spec Peer Review #238

Merged
merged 15 commits into from
Aug 15, 2019
Merged

Initial Spec Peer Review #238

merged 15 commits into from
Aug 15, 2019

Conversation

gakonst
Copy link
Contributor

@gakonst gakonst commented Aug 5, 2019

Copy link
Member

@mhluongo mhluongo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great progress so far. I've got a few nits on the content as well as some open questions that might be worth addressing here or in a new issue

docs/future-work/index.adoc Outdated Show resolved Hide resolved
approaches 100. Note how when we additionally back a deposit with ETH, TBTC
liquidity improves at the expense of capital efficiency.

// full data: https://docs.google.com/spreadsheets/d/1rG9XS6xJbulltwKBMfszfkHeqG5Bl6JboIpIjO1Qn3Q/edit#gid=0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there an easy way to get this data / curve into the repo and out of Google? cc @Shadowfiend

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The curve png helps, just thinking it might be worth pulling a table in directly

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can probably extract a piece to CSV and pull it in as a table; see the example “Table from CSV data in file” in the Table section of the Asciidoctor syntax quick reference. We can also probably shovel that same CSV into TikZ to generate the plot, if we want.

docs/bonding/index.adoc Outdated Show resolved Hide resolved
docs/bonding/index.adoc Outdated Show resolved Hide resolved
docs/bonding/index.adoc Outdated Show resolved Hide resolved
docs/custodial-fees/index.adoc Outdated Show resolved Hide resolved
docs/custodial-fees/index.adoc Outdated Show resolved Hide resolved
docs/custodial-fees/index.adoc Outdated Show resolved Hide resolved
size (`1.0 BTC` initially). This implies that the a user that sends less than `1.0
BTC` in the funding transaction does not receive any tBTC, and forfeits the BTC
locked in the funding transactions to the Signers. The Signers can unlock and
evenly split the funds in the transaction after the _Deposit_ is resolved on-chain (see
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know you didn't do this originally, but let's be consistent on

`Deposit` vs _Deposit_ vs Deposit

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw all 3 used across the document, are they interchangeable? If so we could just go with the italics.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yuck. I know the plaintext treatment is off. Maybe backticks for the capital-D deposit referring directly to a contract, versus italics for lowercase-d?

docs/deposits/index.adoc Show resolved Hide resolved
@gakonst
Copy link
Contributor Author

gakonst commented Aug 10, 2019

Left comments about threshold signing in https://github.com/keep-network/tbtc/pull/243/files

Copy link
Member

@mhluongo mhluongo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good. A couple more nits / other thoughts

docs/custodial-fees/index.adoc Show resolved Hide resolved
docs/signing/index.adoc Outdated Show resolved Hide resolved
docs/signing/index.adoc Outdated Show resolved Hide resolved
docs/signing/index.adoc Outdated Show resolved Hide resolved
docs/future-work/index.adoc Show resolved Hide resolved
docs/future-work/index.adoc Show resolved Hide resolved
docs/future-work/index.adoc Outdated Show resolved Hide resolved
@gakonst gakonst force-pushed the gakonst/spec-peer-review branch from f8e977e to 6bf7149 Compare August 15, 2019 08:01
@gakonst gakonst requested a review from mhluongo August 15, 2019 08:01
@gakonst
Copy link
Contributor Author

gakonst commented Aug 15, 2019

@mhluongo This should be good for one more round of review. Is the tECDSA section elaborate enough? I understand the protocol to add more details, however it might get too verbose if we wanted to write about the multiplicative to additive sharing mechanism along with the commitments / proofs of knowledge required.

Where should we put the graphs from #243?

mhluongo
mhluongo previously approved these changes Aug 15, 2019
Copy link
Member

@mhluongo mhluongo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small nit, but IMO this is ready to merge 🚀 Let's find a spot for the diagram in the redemption section- that can happen in this PR or in a follow-up.

docs/signing/index.adoc Show resolved Hide resolved
@gakonst gakonst dismissed stale reviews from mhluongo via 1bc66ea August 15, 2019 11:51
@mhluongo mhluongo closed this Aug 15, 2019
@mhluongo mhluongo reopened this Aug 15, 2019
mhluongo
mhluongo previously approved these changes Aug 15, 2019
Copy link
Member

@mhluongo mhluongo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💪

@mhluongo
Copy link
Member

mhluongo commented Aug 15, 2019

Uh oh @gakonst, not sure how I missed this- we need all commits signed to merge 😩

The last one looks good, so you be doable via a rebase

@gakonst gakonst force-pushed the gakonst/spec-peer-review branch from ed54737 to a64dc2e Compare August 15, 2019 17:02
Copy link
Member

@mhluongo mhluongo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚢 :shipit:

@mhluongo mhluongo merged commit 2299516 into master Aug 15, 2019
@mhluongo mhluongo deleted the gakonst/spec-peer-review branch August 15, 2019 17:08
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants