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

Stake Distribution, Deposits and Conway #1484

Merged
merged 13 commits into from
Aug 17, 2023
Merged

Conversation

kderme
Copy link
Contributor

@kderme kderme commented Aug 16, 2023

Description

This pr combines 3 different stuff :

These are combined in a single pr, since testing on master any of these 3 tickets is necessary and it's more efficient to test them at once.

Checklist

  • Commit sequence broadly makes sense
  • Commits have useful messages
  • New tests are added if needed and existing tests are updated
  • Any changes are noted in the changelog
  • Code is formatted with fourmolu on version 0.10.1.0 (which can be run with scripts/fourmolize.sh)
  • Self-reviewed the diff

Migrations

  • The pr causes a breaking change of type a,b or c
  • If there is a breaking change, the pr includes a database migration and/or a fix process for old values, so that upgrade is possible
  • Resyncing and running the migrations provided will result in the same database semantically

This pr adds migrations files and is a b type breaking change.
In addition an adjusting procedure is created for the stake distribution changes. On an upgrade this procedure will insert the missing entries that should have been inserted earlier.

@kderme kderme requested review from a team as code owners August 16, 2023 09:06
@kderme kderme changed the title Stake Distribution and Deposits improvements Stake Distribution, Deposits and Conway Aug 16, 2023
@@ -20,7 +20,7 @@ checkForceIndexesArg =
startDBSync dbSyncEnv
-- assertBlockNoBackoff dbSyncEnv 0
threadDelay 3_000_000
assertEqQuery dbSyncEnv DB.queryPgIndexesCount 143 "there wasn't the correct number of indexes"
assertEqQuery dbSyncEnv DB.queryPgIndexesCount 161 "there wasn't the correct number of indexes"
Copy link
Contributor

Choose a reason for hiding this comment

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

these were failing on me today too (they were out by 1) but if I turned on consumed + purge to True they would pass 🤷

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is an additional index when this flag is used, so this is expected

CREATE INDEX IF NOT EXISTS idx_tx_out_consumed_by_tx_in_id ON tx_out (consumed_by_tx_in_id)

Copy link
Contributor

@Cmdv Cmdv left a comment

Choose a reason for hiding this comment

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

LGTM just a couple of questions 👍

{-# LANGUAGE OverloadedStrings #-}
{-# LANGUAGE RankNTypes #-}
{-# LANGUAGE ScopedTypeVariables #-}
{-# LANGUAGE TypeApplications #-}
{-# LANGUAGE TypeFamilies #-}
{-# LANGUAGE TypeOperators #-}
{-# LANGUAGE NoImplicitPrelude #-}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm assuming this is fourmolu, wonder why it keeps putting N after T as you'd think they are ordered alphabetically 🤷

@@ -147,20 +149,29 @@ genericStakeSlice pInfo epoch sliceIndex minSliceSize lstate

-- The starting index of the data in the delegation vector.
index :: Word64
index = sliceIndex * epochSliceSize
index
Copy link
Contributor

Choose a reason for hiding this comment

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

in the future it might be worth splitting these out (index + size) so we can unit test them.

hasConsumed <- liftIO $ getHasConsumed syncEnv
(resolvedInputs, fees', deposits) <- case (mdeposits, unCoin <$> Generic.txFees tx) of
(Just deposits, Just fees) -> do
(resolvedInputs, _) <- splitLast <$> mapM (resolveTxInputs hasConsumed False (fst <$> groupedTxOut grouped)) (Generic.txInputs tx)
Copy link
Contributor

Choose a reason for hiding this comment

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

think splitLast has been deprecated?

Copy link
Contributor

Choose a reason for hiding this comment

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

oh no take it back just seen it later in the review, is it worth having explicit import for it?

cardano-db-sync/src/Cardano/DbSync/Era/Shelley/Insert.hs Outdated Show resolved Hide resolved
cardano-db-sync/src/Cardano/DbSync/Fix/PlutusScripts.hs Outdated Show resolved Hide resolved
@kderme kderme merged commit ebc9aba into master Aug 17, 2023
16 of 67 checks passed
@iohk-bors iohk-bors bot deleted the kderme/stake-dist-master branch August 17, 2023 14:53
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.

Tracking epoch_stake distribution Use Deposits ledger event
2 participants