Skip to content
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

docs: add more on transaction validity #597

Merged
merged 6 commits into from
Mar 10, 2020
Merged

Conversation

paulperegud
Copy link
Contributor

We need more information on transaction validity for referencing from watcher-specs. Example questions implementer might have are:

  • where to look for details?
  • how does it work?

@paulperegud paulperegud requested review from kevsul and pdobacz March 5, 2020 13:09
@paulperegud paulperegud force-pushed the docs-transaction-validity branch from b7d7683 to 488125a Compare March 5, 2020 13:09
Copy link
Contributor

@pdobacz pdobacz left a comment

Choose a reason for hiding this comment

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

Nice! Posted some suggestions.

Can you double check that there isn't any description conflicting/duplicate with the one you've added? I vaguely recall reading something at least partially covering those topics.

Pls scan docs/ in general, if you haven't already.

plasma_framework/docs/integration-docs/integration-doc.md Outdated Show resolved Hide resolved
plasma_framework/docs/integration-docs/integration-doc.md Outdated Show resolved Hide resolved

ALD introduces the concepts of transaction type, and transcation output type. Each transaction type and transcation output type can define different rules about how to spend funds.
ALD introduces the concepts of transaction type and transaction output type.
Each transaction output type can define its own rule about how funds may be spent.
Copy link
Contributor

Choose a reason for hiding this comment

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

its own set of rules

And then I'd add more explanation of the concept of a spending condition, before diving into links to code and "gory details" like the hash part. Like what is the abstraction, that a spending condition is defined per the pair tx_type output_type and so forth.

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've provided an example of how spending condition can be used, before the explanation of how it is implemented.

Copy link
Contributor

Choose a reason for hiding this comment

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

hm, the example is nice (consider choosing sth less speculative though, like a EC signature, which you can then link to our existing Payment tx spending condition). Also mind to keep it to a separate paragraph (check rendered .md)

However, I'd insist on providing some not-an-example explanation of a spending condition concept. Just explain what it is, in 2-3 sentences, one of them could literally be "a spending condition is defined per the pair of a transaction type and an output type".

@boolafish
Copy link
Contributor

boolafish commented Mar 6, 2020

I vaguely recall reading something at least partially covering those topics.

Potentially this, though I think the duplication is quite small. But might have some place to optimize the links in the doc here: https://github.com/omisego/plasma-contracts/blob/master/plasma_framework/docs/design/payment-game-implementation-v1.md#transaction-data-structure

also, if fee is interested: https://github.com/omisego/plasma-contracts/blob/master/plasma_framework/docs/design/fee-game.md#fee-transaction-data-structure

Copy link
Contributor

@kevsul kevsul left a comment

Choose a reason for hiding this comment

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

Looks good, some minor changes for readability

plasma_framework/docs/integration-docs/integration-doc.md Outdated Show resolved Hide resolved
plasma_framework/docs/integration-docs/integration-doc.md Outdated Show resolved Hide resolved
plasma_framework/docs/integration-docs/integration-doc.md Outdated Show resolved Hide resolved
plasma_framework/docs/integration-docs/integration-doc.md Outdated Show resolved Hide resolved
plasma_framework/docs/integration-docs/integration-doc.md Outdated Show resolved Hide resolved
plasma_framework/docs/integration-docs/integration-doc.md Outdated Show resolved Hide resolved
plasma_framework/docs/integration-docs/integration-doc.md Outdated Show resolved Hide resolved
plasma_framework/docs/integration-docs/integration-doc.md Outdated Show resolved Hide resolved
plasma_framework/docs/integration-docs/integration-doc.md Outdated Show resolved Hide resolved
plasma_framework/docs/integration-docs/integration-doc.md Outdated Show resolved Hide resolved
@paulperegud paulperegud force-pushed the docs-transaction-validity branch from d4a249b to 1c4390f Compare March 6, 2020 12:14
@paulperegud paulperegud force-pushed the docs-transaction-validity branch from 1c4390f to 71266c6 Compare March 6, 2020 12:16
@paulperegud paulperegud requested review from kevsul and pdobacz March 6, 2020 12:20
pdobacz
pdobacz previously approved these changes Mar 6, 2020
Copy link
Contributor

@pdobacz pdobacz left a comment

Choose a reason for hiding this comment

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

Just one outstanding comment (left a follow-up to one thread), then LGTM!

@paulperegud paulperegud merged commit fd9146a into master Mar 10, 2020
@paulperegud paulperegud deleted the docs-transaction-validity branch March 10, 2020 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants