Stop using `UMap` in favor of `Accounts` type family #5128

lehins · 2025-06-18T01:34:04Z

Description

Resolves #4694

Checklist

teodanciu

Did a first pass, will have another look later! Looks great, I finally feel I can be friends with DState 😆

teodanciu · 2025-06-25T11:09:19Z

libs/cardano-ledger-core/src/Cardano/Ledger/State/Account.hs

+  -- or not.
+  addAccountState :: Credential 'Staking -> AccountState era -> Accounts era -> Accounts era
+
+  accountsMapL :: Lens' (Accounts era) (Map (Credential 'Staking) (AccountState era))


This doesn't seem to follow the naming convention of the other lenses, how about:
accountStateAccountsL ? too verbose?

No, I don't think this will work, because it is important that it returns a Map.
Every operation that happens after usage of this lens expects a Map.

Also, technically it would have to be plural: either accountStatesAccountsL or accountsStatesAccountsL. In any case, it would be much harder to read it, since there are too many "accounts" in the name 😄 Also, I believe it would be harder to understand what the hell this lens does.

libs/cardano-ledger-core/src/Cardano/Ledger/State/Account.hs

eras/shelley/impl/src/Cardano/Ledger/Shelley/Rules/Deleg.hs

eras/shelley/impl/src/Cardano/Ledger/Shelley/Rules/Delegs.hs

eras/shelley/impl/src/Cardano/Ledger/Shelley/Rules/Deleg.hs

teodanciu · 2025-06-25T14:20:09Z

eras/shelley/impl/src/Cardano/Ledger/Shelley/Rules/PoolReap.hs

+    rewardAccounts_ :: Map.Map (KeyHash 'StakePool) (RewardAccount, Coin)
    rewardAccounts_ = Map.intersectionWith (,) rewardAccounts retiringDeposits
    rewardAccounts' :: Map.Map RewardAccount Coin


These names are terrible, why not something like:

Suggested change

rewardAccounts_ :: Map.Map (KeyHash 'StakePool) (RewardAccount, Coin)

rewardAccounts_ = Map.intersectionWith (,) rewardAccounts retiringDeposits

rewardAccounts' :: Map.Map RewardAccount Coin

rewardAccountsWithCoin :: Map.Map (KeyHash 'StakePool) (RewardAccount, Coin)

rewardAccountsWithCoin = Map.intersectionWith (,) rewardAccounts retiringDeposits

coinsByRewardAccount :: Map.Map RewardAccount Coin

aniketd

Just some minor corrections

eras/conway/impl/src/Cardano/Ledger/Conway/State/Account.hs

libs/cardano-ledger-core/src/Cardano/Ledger/State/CertState.hs

teodanciu

Added a few more minor suggestions

teodanciu · 2025-06-26T12:04:16Z

eras/shelley/impl/src/Cardano/Ledger/Shelley/LedgerState/PulsingReward.hs

-                  rewardAccount
-                  (packageLeaderReward poolRI)
-                  acc
+              || account `Map.member` accountsMap


Also here we could use

Suggested change

|| account `Map.member` accountsMap

| isAccountRegistered account ( ls ^. lsCertStateL . certDStateL . accountsL)

but it's more verbose, so not necessarily worth it, just if we wanted to reinforce the Accounts terminology

This is a nice suggestion, I'll adjust it a tiny bit:

Suggested change

|| account `Map.member` accountsMap

|| isAccountRegistered account accounts

eras/shelley/impl/src/Cardano/Ledger/Shelley/State/Account.hs

teodanciu · 2025-06-26T13:11:23Z

eras/shelley/impl/src/Cardano/Ledger/Shelley/State/Account.hs

+toShelleyAccountsPairs :: Aeson.KeyValue e a => ShelleyAccounts era -> [a]
+toShelleyAccountsPairs sas@(ShelleyAccounts _ _) =
+  let ShelleyAccounts {..} = sas
+   in [ "credentials" .= saStates


Is "credentials" accurate here? Maybe more like "states" ?

I am worried a bit about breakage downstream, but I 100% agree with you that it is a terrible name, so I am very much leaning towards your suggestion of changing it.

eras/shelley/impl/src/Cardano/Ledger/Shelley/State/Stake.hs

eras/shelley/test-suite/src/Test/Cardano/Ledger/Shelley/Rules/Deposits.hs

teodanciu · 2025-06-26T15:47:07Z

eras/shelley/test-suite/test/Test/Cardano/Ledger/Shelley/Examples/Combinators.hs

@@ -158,75 +159,33 @@ newLab b cs =
 --   1) do not give this function duplicates in the 'stakes' or 'pools' inputs.
 --   2) do not use this function when crossing the epoch boundary,
 -- instead use 'newEpoch'.
-feesAndDeposits ::
+addPoolDeposits ::


I don't understand well what this function was doing before, but it looks like the haddock for it no longer matches the implementation. So maybe we can remove it or change it to something like:

Suggested change

addPoolDeposits ::

Add deposits for new pools and adjust the deposit tables in the UTxOState and CertState accordingly.

addPoolDeposits ::

I've fixed the haddock, which hopefully makes it clearer what this function now does:

-- | Add new pools while updating the deposit pot

eras/conway/impl/src/Cardano/Ledger/Conway/State/Account.hs

eras/conway/impl/src/Cardano/Ledger/Conway/Rules/Deleg.hs

teodanciu · 2025-06-26T20:57:02Z

libs/cardano-ledger-api/src/Cardano/Ledger/Api/State/Query.hs

-  (scSPools stakeCredentials, scRewards stakeCredentials)
-  where
-    stakeCredentials = domRestrictedStakeCredentials creds umap
-
 -- | Uses `filterStakePoolDelegsAndRewards` to get the same information from the `NewEpochState`


This comment no longer holds

Suggested change

-- | Uses `filterStakePoolDelegsAndRewards` to get the same information from the `NewEpochState`

libs/cardano-ledger-api/src/Cardano/Ledger/Api/State/Query.hs

teodanciu

That a huge piece of work!! I think it's a great improvement in terms of understanding not only what we store/map but also how to work with it, thanks to the clear interface.

I admit I got lost with the Transition modules, I don't understand at all what they're about, but that's on me, I shall study them.

Looks good to me, great work!

… AccountStates Simplify Transition and reuse it in shelley-test Add mkTestAccountState and addAccountState

Reworked some messages when things fail. Removed some unused imports that were not necessary. Changed Test.Cardano.Ledger.Constrained.Conway.LedgerTypes.Specs, by changing conwayCertStateSpec to pass one component of PState through reify to DState and then pass one component of DState though reify to VState. This replaces the use of two exists. Made some small chages to MiniTrace. Added Witnessing the rng of a map that was ConwayAccountState. Added some Winessed instances that deal with AccountState type family and its instances.

Fix mkConwayTestAccountState which was calling era if the DRep was delegated.

lehins mentioned this pull request Jun 18, 2025

Nested Transactions - CIP-118 #5123

Open

23 tasks

lehins force-pushed the lehins/era-specific-umap branch from 0fa485d to ca95503 Compare June 24, 2025 21:11

teodanciu reviewed Jun 25, 2025

View reviewed changes

aniketd reviewed Jun 26, 2025

View reviewed changes

eras/conway/impl/src/Cardano/Ledger/Conway/State/Account.hs Outdated Show resolved Hide resolved

libs/cardano-ledger-core/src/Cardano/Ledger/State/CertState.hs Outdated Show resolved Hide resolved

teodanciu reviewed Jun 26, 2025

View reviewed changes

libs/cardano-ledger-api/src/Cardano/Ledger/Api/State/Query.hs Show resolved Hide resolved

teodanciu approved these changes Jun 26, 2025

View reviewed changes

lehins force-pushed the lehins/era-specific-umap branch 3 times, most recently from 6c37635 to afaa57f Compare June 28, 2025 02:05

TimSheard force-pushed the lehins/era-specific-umap branch 3 times, most recently from f802c1e to 5eeb3a8 Compare July 2, 2025 12:16

lehins and others added 7 commits July 3, 2025 00:01

Export Maybe expectations

1968be1

Add EitherDeepExpr helpers

753fa2a

Add Uniform instance for Ptr, SlotNo32, CertIx and TxIx

8bf36c5

Viable approach in the works. In process of removing UMap in favor of…

d4a7fee

… AccountStates Simplify Transition and reuse it in shelley-test Add mkTestAccountState and addAccountState

Made a test that required a UMap invalid for eras after Babbage.

f78284b

Fix mkConwayTestAccountState which was calling era if the DRep was delegated.

remove IsNormalType constraint

c45599a

lehins force-pushed the lehins/era-specific-umap branch from 0587e7f to c45599a Compare July 3, 2025 06:01

	\|\| account `Map.member` accountsMap
	\| isAccountRegistered account ( ls ^. lsCertStateL . certDStateL . accountsL)

	\|\| account `Map.member` accountsMap
	\|\| isAccountRegistered account accounts

	addPoolDeposits ::
	Add deposits for new pools and adjust the deposit tables in the UTxOState and CertState accordingly.
	addPoolDeposits ::

Stop using UMap in favor of Accounts type family #5128

Are you sure you want to change the base?

Stop using UMap in favor of Accounts type family #5128

Conversation

lehins commented Jun 18, 2025

Description

Checklist

Uh oh!

teodanciu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

teodanciu Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aniketd left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

teodanciu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

teodanciu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Stop using `UMap` in favor of `Accounts` type family #5128

Stop using `UMap` in favor of `Accounts` type family #5128

teodanciu Jun 25, 2025 •

edited

Loading

aniketd left a comment •

edited

Loading