-
Notifications
You must be signed in to change notification settings - Fork 20
[Dijkstra] TxBody replace txRequiredGuards
#1008
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
[Dijkstra] TxBody replace txRequiredGuards
#1008
Conversation
This closes issue #1000.
and add TODO/reminder for `txRequiredTopLevelGuards` type change
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.
Pull request overview
This PR refactors the Dijkstra transaction specification to properly separate the concepts of "signers" and "guards" in accordance with CIP-0112/0118. The key change is renaming txRequiredGuards to txGuards and upgrading its type from ℙ KeyHash to ℙ Credential to support both key and script credentials. A new reqSignerHashes field is added to maintain backward compatibility for phase-1/native script validation.
- Renamed
TxBody.txRequiredGuardstoTxBody.txGuardswith upgraded typeℙ Credential - Added
TxBody.reqSignerHashes : ℙ KeyHashfor phase-1 script signers - Updated
TxInforecord andtxInfofunctions to map the new fields correctly
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/Ledger/Dijkstra/Specification/Transaction.lagda.md | Updates TxBody record with renamed txGuards field and new reqSignerHashes field |
| src/Ledger/Dijkstra/Specification/Script/Validation.lagda.md | Updates TxInfo record to include both vkKey and txGuards fields, and updates txInfo function implementations to map from the new TxBody fields |
| CHANGELOG.md | Documents the semantic changes to TxBody field names and additions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
TxBody replace txRequiredGuardsTxBody replace txRequiredGuards
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.
I'm not sure this is the right way to go. Decoupling reqSignerHashes from txGuards has a couple of drawbacks:
- The
CDDLspecification in CIP-0112, states that inTxBody,required_signersfield is replaced byrequired_observers(orguardsin nested transactions nomenclature). - There would be semantic duplication between phase-1 scripts contained in
txRequiredGuardsand those contained inreqSignerHashes.
Nice observation. You're exactly right on both points. If we kept both As simple fix is to
As for
but the key point is that |
|
One more remark about this: maybe the intention of the CIP is that we need "actual signatories" for UPDATE. Actually, I don't think that's right. I think (in Conway at least) the thing modeled by When we implement Dijkstra phase-1/timelock/native-script checking, we'll still want a notion of actual witnessed signers (from the witness set / vkey signatures)... but I don't think we'll use |
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! Minor comments here and there.
| -- | ||
| txRequiredGuards : ℙ KeyHash -- replaces reqSigHash : ℙ KeyHash | ||
| txSubTransactions : InTopLevel txLevel (List (Tx TxLevelSub)) -- only in top-level tx | ||
| txGuards : ℙ Credential -- Dijkstra guards: credentials required by this tx (key or script). |
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'd make this comment part of an admonition that explains how Dijkstra transaction differs from Conway (at the (sub/top) transaction level, not at the level of nested transactions)
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've moved the comments into the prose above the definition.
| ; txCerts = TxBody.txCerts txBody | ||
| ; txWithdrawals = TxBody.txWithdrawals txBody | ||
| ; txVldt = TxBody.txVldt txBody | ||
| ; vkKey = TxBody.reqSignerHashes txBody |
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.
Probably this field will have to change to requiredGuards in accordance.
But for now I wouldn't worry too much about txInfo.
Description
This closes issue #1000.
This PR addresses two issues
CIP-0112 upgrades required signers to a set of credentials (key or script), but we currently use
ℙ KeyHash, which cannot represent required guard scripts.We should intentionally model signers and guards as distinct concepts: signers feed phase-1/native scripts, guards are the new Dijkstra (CIP-0112/0118) execution requirements.
by doing the following:
Introduce/rename
TxBodyfield (previously calledtxRequiredGuards) present at both top- and sub-levels:txGuards : … Credential …(container tbd:List,OSet, etc)Replace all uses of
txRequiredGuardsin Dijkstra with the new credential container.Keep
TxInfo.vkKeyas a key-only view derived fromtxGuards(CIP-0112's required_observers).reqSignerHashesfromTxBody.reqSignerHashes : ℙ KeyHashhelper/definition (projection fromtxGuards).TxBody.reqSignerHasheswith that helper.TxInfo.vkKeyas a key-only view derived fromtxGuards. This avoids having two independentTxBodyfields representing overlapping requirements. (See Carlos' comment.)Reason. Even in Dijkstra, we still need the old "required signers / signatories" concept for:
vKeySigs)validP1Script : ℙ KeyHash → ... → P1Script → Typestill needs aℙ KeyHash)TxInfo.vkKey : ℙ KeyHashfield aligns with/needs a "key hashes that count as signatories"Copilot-generated Description (manually reviewed and revised)
This pull request introduces several updates to the Dijkstra specification, focusing on transaction guard handling and the transaction info structure. The changes standardize how required signers and guards are represented and accessed, and update the terminology and field names for clarity and alignment with recent proposals. Additionally, some minor documentation and formatting improvements are included.
Transaction guard and signer handling:
TxBody.txRequiredGuardsfield toTxBody.txGuardsand changed its type toℙ Credential, representing credentials (either key or script) required by the transaction.TxBody.reqSignerHashesfrom an (independent) field to a (dependent) function, which derives key hashes from theTxBody.txGuardsfield.TxInforecord and thetxInfofunction to use the newtxGuardsfield andreqSignerHashesfunction, ensuring that both native/phase-1/timelock signers and CIP-0112/0118 guards are represented.Documentation and specification updates:
txRequiredGuardstotxGuardsand the addition ofreqSignerHashes.Minor formatting fixes:
Checklist
CHANGELOG.md