-
Notifications
You must be signed in to change notification settings - Fork 23
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
Refactor TxMintValue
#663
base: master
Are you sure you want to change the base?
Refactor TxMintValue
#663
Conversation
8b36408
to
ef2b00c
Compare
ef2b00c
to
c2c740b
Compare
555e92d
to
6e860f9
Compare
6e860f9
to
fefbe40
Compare
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 looks good to me. One gripe regarding traverseScriptWitnesses
. Clean up and I'll have a final look before approving.
, (assetName', quantity, _) <- assets | ||
] | ||
|
||
-- | Index the assets with witnesses in the order of policy ids. |
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.
Can you add a comment here referencing the spec and why we have to index on the order of policy ids?
@@ -1588,7 +1619,7 @@ data TxBodyError | |||
| TxBodyOutputNegative !Quantity !TxOutInAnyEra | |||
| TxBodyOutputOverflow !Quantity !TxOutInAnyEra | |||
| TxBodyMetadataError ![(Word64, TxMetadataRangeError)] | |||
| TxBodyMintAdaError | |||
| TxBodyMintAdaError -- TODO remove - case nonexistent |
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.
👍
|
||
traverseScriptWitnesses | ||
:: [(a, Either (TxBodyErrorAutoBalance era) (ScriptWitness ctx era))] | ||
-> Either (TxBodyErrorAutoBalance era) [(a, ScriptWitness ctx era)] | ||
:: [(a, Either (TxBodyErrorAutoBalance era) b)] |
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 may be "neater" Haskell but making this more generic doesn't really help the reader IMO. Also it's only instantiated to ScriptWitness ctx era
.
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.
It's not only instantiated to ScriptWitness ctx era
- in line 1629 is used against
[ (Value, Either (TxBodyErrorAutoBalance era)
( AssetName
, Quantity
, BuildTxWith build (ScriptWitness WitCtxMint era)
)
]
(Map PolicyId (ScriptWitness WitCtxMint era)) | ||
-> Map | ||
PolicyId | ||
[ ( AssetName |
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.
A 3-tuple is less than ideal but I can't think of what to call this instead.
@@ -676,6 +677,17 @@ data WitCtx witctx where | |||
WitCtxMint :: WitCtx WitCtxMint | |||
WitCtxStake :: WitCtx WitCtxStake | |||
|
|||
-- TODO: not needed anymore - remove |
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.
👍
…WitnessReferenceInputOrScript functions
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