-
Notifications
You must be signed in to change notification settings - Fork 20
Dijkstra: nested transactions (first phase) #942
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
Conversation
carlostome
left a comment
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.
LGTM! I left some comments here and there.
| ## Changes to Transaction Validity | ||
|
|
||
| As discussed in [Ledger.Conway.Specification.Properties][], transaction validity is | ||
| tricky, and this is as true in the Dijkstra era as it was in Conway, if not moreso. | ||
|
|
||
| Here are some key points about transaction validity in the Dijkstra era. | ||
|
|
||
| 1. Sub-transactions are not allowed to contain sub-transactions themselves. | ||
|
|
||
| 2. Sub-transactions are not allowed to contain collateral inputs. Only a top-level | ||
| transaction is allowed to (furthermore, obligated to) provide sufficient | ||
| collateral for all scripts that are run as part of validating all transactions in | ||
| that batch. If any script in a batch fails, none of the transactions in the batch | ||
| is applied; only the collateral is collected. |
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.
How is this relevant to script validation?
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.
Okay, I moved this discussion of transaction validity to the bottom of Ledger.Dijkstra.Specification.Transaction, but please suggest a more appropriate place if you see one.
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 this will go in Ledger since is about the top level behaviour.
| ## Transaction levels | ||
|
|
||
| To differentiate between the two types of transactions (i.e. top-level | ||
| and sub), we define the type of transaction level: |
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.
| and sub), we define the type of transaction level: | |
| and sub-level), we define the type of transaction level: |
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.
Hmmm... I thought, since we call them "top-level transactions" and "sub-transactions," it is appropriate to say, "the two types of transactions (i.e., top-level and sub)..." On the other hand, I suppose it's also reasonable to sometimes use "sub-level transaction" to refer to a sub-transaction. 🤔 Wdyt?
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 just find a bit weird to use top-level but sub. But also it will be weird to use top and sub (top w/o level).
402a6e1 to
eb330d3
Compare
|
@carlostome thanks for all of your suggestions. I've incorporated all except for the two mentioned above. Please could you have one more quick look over the PR and let me know if you still approve? |
+ Rename Observers to Guards + Rename Lvl to Level; use keyword 'mutual' + Add some text and comments + Expand code region + Add more explanations + Add headers + Reword and add more text + fix issues caused by merge of master
+ add and type-check some new modules: + Ledger.Dijkstra + Ledger.Dijkstra.Specification + Ledger.Dijkstra.Specification.Script.Validation + revert getCoin rename + remove unused + corrections
c36092e to
bffe14d
Compare
carlostome
left a comment
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.
LGTM!
Description
Initial PR for formalizing nested transactions in the Dijkstra era.
Checklist
CHANGELOG.md