-
Notifications
You must be signed in to change notification settings - Fork 15
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
Remove unneeded ScriptHash in PReferenceScript/SReferenceScript #959
base: master
Are you sure you want to change the base?
Conversation
4b665a1
to
edbad42
Compare
cardano-cli/test/cardano-cli-golden/files/golden/mary/transaction-view.out
Outdated
Show resolved
Hide resolved
edbad42
to
e797b17
Compare
e797b17
to
c461679
Compare
-> [(PolicyId, ScriptWitness WitCtxMint era)] | ||
gatherMintingWitnesses [] = [] | ||
gatherMintingWitnesses (sWit : rest) = | ||
case scriptWitnessPolicyId sWit of | ||
gatherMintingWitnesses ((mPid, sWit) : rest) = |
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 need a new data definition.
fetchReferenceMintingScriptWitness :: UTxO era -> ScriptWitness WitCtxMint era -> UpdatedReferenceScriptWitness
and in the case where the user chooses to supply it
updateReferenceScriptWitness :: PolicyId -> ScriptWitness WitCtxMint era -> UpdatedReferenceScriptWitness
Propagating Maybe PolicyId
may work but it results in poor readability. If the user does not supply the policy id we want the ability to query the UTxO to get the reference script witnesses so we can calculate the policy ID.
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.
This would be the short to medium term fix until we can figure out how to encapsulate this in cardano-api
.
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 agree about the first case with fetchReferenceMintingScriptWitness
. I'd do this in a follow-up PR.
What is not clear to me is how to thread in Maybe PolicyId
from the CLI argument parser to this place in a clean way. What's your idea about this scenario, when the user provides Maybe PolicyId
manually?
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 would start with something like: data MintingReferenceScriptWitness era = MintingReferenceScriptWitness PolicyId (ScriptWitness WitCtxMint era)
I'd do this in a follow-up PR.
In this instance I would insist its done in this PR.
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.
No worries, let me push some changes tomorrow to make sure what I am suggesting can actually work.
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 removed my comment as I'm still wrapping my head around the idea and experimenting with it. Feel free to push.
eab98ec
to
37c99ec
Compare
37c99ec
to
1450d80
Compare
-> ( TxIn | ||
-> ExceptT |
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.
-> ( TxIn | |
-> ExceptT | |
-> ( Set TxIn | |
-> ExceptT |
we can have multiple txins, so for performance reasons this should be one query for all witnesses
Changelog
Context
Additional context for the PR goes here. If the PR fixes a particular issue please provide a link to the issue.
How to trust this PR
Highlight important bits of the PR that will make the review faster. If there are commands the reviewer can run to observe the new behavior, describe them.
Checklist