Skip to content
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

more events, AnnotatedBlock #2437

Closed
wants to merge 8 commits into from
Closed

more events, AnnotatedBlock #2437

wants to merge 8 commits into from

Conversation

goolord
Copy link
Contributor

@goolord goolord commented Aug 26, 2021

No description provided.

@nc6
Copy link
Contributor

nc6 commented Aug 30, 2021

What's the logic behind AnnotatedBlock? Having another top-level function to call is annoying, because it requires plumbing all the way through the system.

@goolord
Copy link
Contributor Author

goolord commented Aug 30, 2021

theres discussion here IntersectMBO/cardano-db-sync#668 and AnnotatedBlock seemed relatively uncontroversial. I think it could probably be an event or something so we don't need to do all of this plumbing again

@nc6
Copy link
Contributor

nc6 commented Aug 31, 2021

Yep, having this data as an event makes lots of sense. I'd rather avoid the current implementation.

@goolord goolord force-pushed the db-sync-wishlist-2 branch 2 times, most recently from 558b335 to a891924 Compare September 27, 2021 13:30
@goolord goolord force-pushed the db-sync-wishlist-2 branch 3 times, most recently from 963bd9a to 0fcdf2b Compare October 11, 2021 22:34
@goolord goolord force-pushed the db-sync-wishlist-2 branch from 0fcdf2b to 1a0caf2 Compare October 12, 2021 16:43
@goolord goolord requested review from nc6 and JaredCorduan October 12, 2021 16:43
@nc6
Copy link
Contributor

nc6 commented Nov 15, 2021

This somehow has a very large number of unrelated changes integrated into it... please can you rebase just the relevant parts onto master?

@goolord
Copy link
Contributor Author

goolord commented Nov 15, 2021

oh yeah, i don't know what's up with that. i'll fix that

Copy link
Contributor

@nc6 nc6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I can see why you've taken this approach, but I think it's going to be better to have these as conventional events, each generated at the correct place. So we would have a bunch of AnnotatedTx events, plus a block-level one.

data AnnotatedTx = AnnotatedTx
{ atInputSum :: !Coin, -- Sum of the tx inputs
atOutSum :: !Coin, -- Sum of the tx outputs
txFees :: !Coin -- All fields in the superset of all tx types
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Speaking with Erik, he would also like to have the actual resolved inputs here.

@@ -64,7 +84,8 @@ class
Environment (Core.EraRule "BBODY" era) ~ STS.BbodyEnv era,
State (Core.EraRule "BBODY" era) ~ STS.BbodyState era,
Signal (Core.EraRule "BBODY" era) ~ (Block BHeaderView era),
ToCBORGroup (TxSeq era)
ToCBORGroup (TxSeq era),
Coercible (Event (Core.EraRule "BBODY" era)) (STS.BbodyEvent era)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I appreciate this is more general, but for consistency with everything else I'd rather stick with the equality constraint.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some instances won't compile if this is in equality constraint. This constraint will get floated outside of typeclass. Maybe it could be an Embed constraint instead, but the Coercible constraint is not superficially more general

abEpochSlot :: !SlotNo, -- The slot within the epoch (starts at 0 for first slot of each epoch
abTimeStamp :: !UTCTime, -- The slot number converted to UTCTime
abEpochSize :: !EpochSize, -- Number of slots in current epoch
abTxs :: [AnnotatedTx] -- All fields in the superset of all block types
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't understand the comment here

Copy link
Contributor Author

@goolord goolord Nov 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's from https://github.com/input-output-hk/cardano-db-sync/blob/54852e7981da92c3ba089d3863977eeffdb862e6/doc/ledger-consensus-api.md

the AnnotatedTx is meant to contain fields from the TxIn/TxOut/TxBody of each i believe

eras/shelley/impl/src/Cardano/Ledger/Shelley/Rules/Snap.hs Outdated Show resolved Hide resolved
pools = _pParams $ _pstate $ _delegationState $ ledgerState
certs = Foldable.toList (getField @"certs" txBody)
ins =
Val.coin (balance @era (eval ((getField @"inputs" txBody) <| _utxo u)))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doing this within the API, rather than during the appropriate ledger rule, means we end up resolving the inputs twice. The correct place for this would surely be in the Utxo rule?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this expensive? I can't imagine this AnnotatedBlock would be very useful if it was split up into several events. I don't see where in Cardano.Ledger.Alonzo.Rules.Utxo.hs would be more appropriate, I feel like I'm missing something

@goolord goolord closed this Feb 10, 2022
@goolord
Copy link
Contributor Author

goolord commented Feb 10, 2022

closed in favor of #2652

@JaredCorduan JaredCorduan deleted the db-sync-wishlist-2 branch August 16, 2022 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants