Audit changes #445
Audit changes #445
Conversation
|
I think we should keep this branch as a reference but when we decide to apply the changes to main after our audit obligations, we should open one PR per feature these commits cover, then review, merge, and iterate like this. There is really a lot here and each subject would likely benefit from a dedicated PR (whether it is tiny or large), with discussion and review (whether it is trivial or turns into debates), and maybe small tweaks. Furthermore, it would be easier to search through the PR tracker rather than commit messages if in the future we want to remember what we changed, when, and why. |
There was a problem hiding this comment.
This is a review of only commit db84394 "Support for hashed datums in reference inputs".
This is only extreme nitpicking stuff which may be ignored altogether.
Since this is a per commit review, the suggestion GUI was not available.
src/Cooked/MockChain/BlockChain.hs
Outdated
| let outputToDatumHashM output = case output ^. outputDatumL of | ||
| Api.OutputDatumHash dHash -> Just dHash | ||
| _ -> Nothing |
There was a problem hiding this comment.
Is it something useful elsewhere that we should put on top level?
There was a problem hiding this comment.
Something that could be reused was already existing indeed and I now used it: isOutputWithDatumHash. Thanks!
src/Cooked/MockChain/BlockChain.hs
Outdated
| -- | Looks us the data on UTxOs the transaction consumes. This corresponds to | ||
| -- the keys of what should be removed from the stored datums in our mockchain. | ||
| -- There can be duplicates, which is expected. | ||
| txSkelConsumedData :: (MonadBlockChainBalancing m) => TxSkel -> m [Api.DatumHash] |
There was a problem hiding this comment.
I find the names confusing. This ends with Data and returns a list of hashes, the previous one ends with HashedData and returns a map that resolve to the actual data.
Also I find "consumed" a bit misleading. This is the same as "spent"? This means the datum hashes of every piece of datum found in the inputs, doesn't it? Not a subset.
There was a problem hiding this comment.
I changed the name to txSkelInputDataAsHashes
| <+> "(" | ||
| <> prettyHash (pcOptHashes opts) (toHash dHash) | ||
| <> "):" |
There was a problem hiding this comment.
Maybe we should have a flag in the prettyprinting options to enable this on demand and hide it by default. The default printing might be starting to get a bit crowded (this is not the reason, this is just a drop, it just makes me think about it). In general, we don't really track datum hashes and this would be lighter by default.
For instance pcOptPrintDatumHashes like there already is a pcOptPrintTxHashes, false by default.
There was a problem hiding this comment.
I will leave these changes to later changes in the pretty printer.
| @@ -59,6 +60,11 @@ instance Transform TxContext Input.InputContext where | |||
| instance Transform TxContext Collateral.CollateralContext where | |||
| transform TxContext {..} = Collateral.CollateralContext {..} | |||
|
|
|||
| instance Transform TxContext Withdrawals.WithdrawalsContext where | |||
There was a problem hiding this comment.
I am a bit skeptical about this Transform typeclass. This is a whole topic on its own, not related to this here specifically, so I won't comment on this here anymore. We may discuss it between maintainers or open the discussion in an issue.
This PR contains a variety of changes/bugfixes that were needed during recent work relying on cooked-validators:
MockChainSthas been extended and given its own module