-
Notifications
You must be signed in to change notification settings - Fork 87
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
plutus: 1.9 -> 1.15.0.1, cardano-api: 8.20 -> 8.29.1 #1168
Conversation
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.
Let's try to only tie hydra-cardano-api
to cardano-api
and insulate the remaining code base as much as possible. See my comments for some ideas.
hydra-cardano-api/src/Hydra/Cardano/Api/MultiAssetSupportedInEra.hs
Outdated
Show resolved
Hide resolved
hydra-cardano-api/src/Hydra/Cardano/Api/ReferenceTxInsScriptsInlineDatumsSupportedInEra.hs
Outdated
Show resolved
Hide resolved
hydra-cardano-api/src/Hydra/Cardano/Api/ScriptDataSupportedInEra.hs
Outdated
Show resolved
Hide resolved
body <- createAndValidateTransactionBody bodyContent | ||
let witnesses = [makeShelleyKeyWitness body (WitnessPaymentKey sk)] | ||
body <- createAndValidateTransactionBody BabbageEra bodyContent | ||
let witnesses = [makeShelleyKeyWitness ShelleyBasedEraBabbage body (WitnessPaymentKey sk)] |
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.
Commenting once, but applies to multiple places:
Avoid mentioning Babbage
in packages outside of hydra-cardano-api
. Currently, that package serves as an "anti-corruption" layer and hard-codes the era we target. Even if we never make it configurable, we want to limit the amount of code that needs changing. That's also the reason why we have all those patterns and alias functions in Hydra.Cardano.Api
, which remove the need for call sites to pick an era.
Must: try to avoid "witness" values outside of hydra-cardano-api
.
Concretely telling from this call site, I would propose to provide a fixed-era createAndValidateTransactionBody :: TxBodyContent Era -> Either TxBodyError (TxBody Era)
in hydra-cardano-api
and the same for makeShelleyKeyWitness
and/or using type class constraints. Note the Tx
return value of mkSimpleTx
is a hard-coded Tx Era = Tx BabbageEra
right now.
036195e
to
d21fa60
Compare
d21fa60
to
d1e51a7
Compare
eb28a6b
to
fb5378d
Compare
Seems like there is some "funny" test errors which are hard to troubleshoot: https://github.com/input-output-hk/hydra/actions/runs/7045153258/job/19174290641?pr=1168#step:5:1856 |
ee567e9
to
76946a2
Compare
6f4866f
to
cbb1ab0
Compare
Transactions CostsSizes and execution budgets for Hydra protocol transactions. Note that unlisted parameters are currently using
Script summary
Cost of Init Transaction
Cost of Commit TransactionThis is using ada-only outputs for better comparability.
Cost of CollectCom Transaction
Cost of Close Transaction
Cost of Contest Transaction
Cost of Abort TransactionSome variation because of random mixture of still initial and already committed outputs.
Cost of FanOut TransactionInvolves spending head output and burning head tokens. Uses ada-only UTxO for better comparability.
End-To-End Benchmark ResultsThis page is intended to collect the latest end-to-end benchmarks results produced by Hydra's Continuous Integration system from the latest Please take those results with a grain of salt as they are currently produced from very limited cloud VMs and not controlled hardware. Instead of focusing on the absolute results, the emphasis should be on relative results, eg. how the timings for a scenario evolve as the code changes. Generated at 2023-12-06 16:42:58.932034475 UTC Baseline Scenario
Baseline Scenario
|
c1de8d0
to
7d6548e
Compare
7d6548e
to
8fd95c7
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 is looking good now! Only a few suggestions and one commit to drop before we merge this.
Please update the PR title and provide a description why this is needed / how it is related to some issue (I think it is needed, but PRs or commits should be documenting this)
Also, please honor the PR template and update at least the CHANGELOG.
cabal.project
Outdated
, hackage.haskell.org 2023-10-20T06:54:18Z | ||
, cardano-haskell-packages 2023-10-20T16:16:59Z | ||
, hackage.haskell.org 2023-12-27T06:54:18Z | ||
, cardano-haskell-packages 2023-12-27T16:16:59Z |
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.
Should: use actual index dates to remove warnings printed
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 is the same issue as it was with the benchmarks. The fact that we are persisting JSON serialized transactions here which can change their byte representation (which hashed gives the transaction id
) is fragile.
No need to change something in this PR, but this is yet another argument against having JSON of transactions and maybe only allow CBOR encoding of them (+ prepare / select tooling to easily introspect transaction CBOR)
211056c
to
b21aae6
Compare
cardano-api: 8.20 -> 8.29.1
It seems like ledger changed some metada encoding between versions. We had some specific auxiliaryData re-encoding fail, while most of the golden data was roundtripping just fine. We suspect a slight difference in the "canonical" serialization produced by the ledger **iff** a data type was not serialized before. Which is the case in our golden test suite where we generated random values and serialize them. TLDR; our golden tests are quite strict and this should be fine
This ensures that the byte-for-byte decoded data set is ingested into a Hydra head correctly. The JSON instance of transactions might not preserve the body byte-for-byte and hence invalidate witnesses leading to invalid transaction.
Seems like we are having issues with inconsistent To/FromJSON serialisation following upgrade, so perhaps we should move our logs to use the "canonical" CBOR represenation of a Tx instead of semi-custom JSON?
This was renamed upstream and we should capture the meaning the same way. It is to provide witnesses for features which are only available from mary era onwards.
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.
Addressed all my concerns and this can be merged now IMO
Same rationale as previous commit, got renamed upstream.
Same rationale as previous two commits
These re-definitions are to avoid additional witness passing as long as we are just fine with the one specific / latest era (which we are by using Hydra.Cardano.Api). Consequently and according to the package README, these belong to the Api.hs module.
This avoids mentioning "Babbage" in downstream packages like hydra-cluster and will reduce the number of places to switch.
This avoids mentioning "Babbage" in downstream packages like hydra-cluster and will reduce the number of places to switch.
This reduces the number of times Babbage is mentioned in the hydra-node package and will help switching eras.
This removes "Requested index-state .. is newer than" warnings when building with cabal.
No description provided.