-
Notifications
You must be signed in to change notification settings - Fork 88
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
cardano-api: 8.29.1.0 -> 8.37.0.0 #1232
Conversation
locallycompact
commented
Jan 3, 2024
•
edited
Loading
edited
- CHANGELOG updated or not needed
- Documentation updated or not needed
- Haddocks updated or not needed
- No new TODOs introduced or explained herafter
ce78b97
to
3404f9e
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 2024-02-02 13:33:01.039541148 UTC Baseline Scenario
Baseline Scenario
|
3404f9e
to
af7ad25
Compare
We consider this blocked by #1213 as this would make us not need to work around a changed |
0a69b7e
to
55a9077
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.
Had some questions, overall looks good 👍
-- | ||
-- NOTE: this function is partial and throws if given a Byron transaction for | ||
-- which fees are necessarily implicit. | ||
txFee' :: HasCallStack => Tx era -> Lovelace |
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.
Why don't we keep the HasCallStack
here?
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.
Because this function seemingly can be made total now
@@ -7,8 +7,9 @@ import Hydra.Cardano.Api.MaryEraOnwards (IsMaryEraOnwards (..)) | |||
-- | Inject some 'Value' into a 'TxOutValue' | |||
mkTxOutValue :: | |||
forall era. | |||
IsShelleyBasedEra era => | |||
IsMaryEraOnwards 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.
Do we need to keep both constraints here?
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.
Yes. Neither implies the other
@@ -331,8 +337,16 @@ queryUTxOByTxIn :: | |||
queryUTxOByTxIn networkId socket queryPoint inputs = | |||
runQueryExpr networkId socket queryPoint $ do | |||
(AnyCardanoEra era) <- queryCurrentEraExpr | |||
eraUTxO <- queryInEraExpr era $ QueryUTxO (QueryUTxOByTxIn (Set.fromList inputs)) | |||
pure $ UTxO.fromApi eraUTxO | |||
(sbe :: ShelleyBasedEra e) <- liftIO $ assumeShelleyBasedEraOrThrow 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.
Is this type annotation needed?
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 should work without it.
6fa48f3
to
8404d4d
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.
Overall fine changes, but we should avoid adding a witness to fromApi
when we have type classes and also adding always 10ADA to genValue
is dangerous IMO.
-- | ||
-- NOTE: this function is partial and throws if given a Byron transaction for | ||
-- which fees are necessarily implicit. | ||
txFee' :: HasCallStack => Tx era -> Lovelace |
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.
Because this function seemingly can be made total now
(IsMaryEraOnwards era, IsAlonzoEraOnwards era, IsBabbageEraOnwards era, IsShelleyBasedEra era) => | ||
Network -> | ||
Plutus.TxOut -> | ||
Maybe (TxOut CtxUTxO era) | ||
fromPlutusTxOut network out = do | ||
value <- TxOutValue maryEraOnwards <$> fromPlutusValue plutusValue | ||
value <- shelleyBasedEraConstraints (shelleyBasedEra @era) $ TxOutValueShelleyBased (shelleyBasedEra @era) <$> (toLedgerValue (maryEraOnwards @era) <$> fromPlutusValue plutusValue) |
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 mkTxOutValue
This seemingly has become a lot more complicated now (too complicated? smell?) and hence we should keep the complexity in one function only.
@@ -46,7 +46,7 @@ mkTxOutAutoBalance :: | |||
ReferenceScript Era -> | |||
TxOut CtxTx Era | |||
mkTxOutAutoBalance pparams addr val dat ref = | |||
let out = TxOut addr (TxOutValue maryEraOnwards val) dat ref | |||
let out = TxOut addr (TxOutValueShelleyBased (shelleyBasedEra @Era) (toLedgerValue (maryEraOnwards @Era) val)) dat ref |
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 mkTxOutValue
This seemingly has become a lot more complicated now (too complicated? smell?) and hence we should keep the complexity in one function only.
@@ -1,14 +1,13 @@ | |||
module Hydra.Cardano.Api.Value where | |||
|
|||
import Hydra.Cardano.Api.Prelude | |||
import Hydra.Cardano.Api.Prelude hiding (toLedgerValue) |
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 the upstream toLedgerValue
if it exists now
@@ -159,7 +158,7 @@ chainSyncClient tracer networkId startingPoint observerHandler = | |||
ClientStNext | |||
{ recvMsgRollForward = \blockInMode tip -> ChainSyncClient $ do | |||
case blockInMode of | |||
BlockInMode _ (Block _header txs) BabbageEraInCardanoMode -> do | |||
BlockInMode BabbageEra (Block _header txs) -> do |
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.
Nice.. they removed some redundancy witnessing.
@@ -87,35 +87,35 @@ min = UTxO . uncurry Map.singleton . Map.findMin . toMap | |||
-- * Type Conversions | |||
|
|||
-- | Transforms a UTxO containing tx outs from any era into Babbage era. | |||
fromApi :: Cardano.Api.UTxO era -> UTxO | |||
fromApi (Cardano.Api.UTxO eraUTxO) = | |||
fromApi :: forall era. ShelleyBasedEra era -> Cardano.Api.UTxO era -> UTxO |
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.
Must: not introduce witness values if we can avoid it
We do not need this witness value and we try hard to stick to a constraint based interface in hydra-cardano-api
.
fromApi :: forall era. ShelleyBasedEra era -> Cardano.Api.UTxO era -> UTxO | |
fromApi :: IsShelleyBasedEra era => Cardano.Api.UTxO era -> UTxO |
should be possible here.
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 don't see how to do this. We are querying AnyCardanoEra at runtime. How do you prove this constraint holds?
|
||
assumeShelleyBasedEraOrThrow :: MonadThrow m => CardanoEra era -> m (ShelleyBasedEra era) | ||
assumeShelleyBasedEraOrThrow era = do | ||
x <- requireShelleyBasedEra 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.
Could: name this requireShelleyBasedEraOrThrow
to indicate it's a thin wrapper around requireShelleyBasedEra
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.
Could: follow the module style and use an Maybe
discharging function like throwOnUnsupportedNtcVersion
for Either
i.e. throwOnNonShelleyBasedEra :: MonadThrow m => Maybe (ShelleyBasedEra a) -> m (ShelleyBasedEra a)
@@ -331,8 +337,16 @@ queryUTxOByTxIn :: | |||
queryUTxOByTxIn networkId socket queryPoint inputs = | |||
runQueryExpr networkId socket queryPoint $ do | |||
(AnyCardanoEra era) <- queryCurrentEraExpr | |||
eraUTxO <- queryInEraExpr era $ QueryUTxO (QueryUTxOByTxIn (Set.fromList inputs)) | |||
pure $ UTxO.fromApi eraUTxO | |||
(sbe :: ShelleyBasedEra e) <- liftIO $ assumeShelleyBasedEraOrThrow 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 should work without it.
c7c1321
to
2fe512a
Compare
2fe512a
to
4984495
Compare
This works as long as all shelley based eras use the 'MaryValue era' type from the cardano-ledger.
9054b74
to
288d5db
Compare
59c5752
to
82a47a5
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.
Still some "Should"-haves, but in principle I do approve now (as we explored the situation with genValue
and know why things are one way or the other now)
@@ -428,7 +429,7 @@ genAddressInEra networkId = | |||
mkVkAddress networkId <$> genVerificationKey | |||
|
|||
genValue :: Gen Value | |||
genValue = scale (`div` 10) $ fromLedgerValue <$> arbitrary | |||
genValue = liftA2 (<>) (pure $ lovelaceToValue $ Lovelace 10_000_000) (scale (`div` 10) $ fromLedgerValue <$> arbitrary) |
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: not need to divide by 10
anymore and use a bit smaller value, 10ADA is a bit much and I think 1-2 ADA is "more realistic" (I know it was millions of ADA before)