5191 Rename PoolParams #5211

kayvank · 2025-08-05T00:41:45Z

Rename PoolParams to StatekPoolParams, to avoid confusion with protocol parameters

Description

This PR is for issue #5191, Rename PoolParams to StatekPoolParams, to avoid confusion with protocol parameters

Checklist

[x ] Commits in meaningful sequence and with useful messages.
Tests added or updated when needed.
CHANGELOG.md files updated for packages with externally visible changes.
NOTE: New section is never added with the code changes. (See RELEASING.md).
Versions updated in .cabal and CHANGELOG.md files when necessary, according to the
versioning process.
Version bounds in .cabal files updated when necessary.
NOTE: If bounds change in a cabal file, that package itself must have a version increase. (See RELEASING.md).
Code formatted (use scripts/fourmolize.sh).
Cabal files formatted (use scripts/cabal-format.sh).
CDDL files are up to date (use scripts/gen-cddl.sh)
hie.yaml updated (use scripts/gen-hie.sh).
Self-reviewed the diff.

kayvank · 2025-08-05T00:44:18Z

PR is in draft mode pending 3 more changes

This PR made changes to many directories; thus, the corresponding CHANGELOG for all those sub-projects needs to be updated, yes?
We should also change the module name from Cardano.Ledger.PoolParams to Cardano.Ledger.StakePoolParams yes?
StatkePoolParams data members should start with sp, yes?

Rename PoolParams to StatekPoolParams, to avoid confusion with protocol parameters

kayvank · 2025-08-05T11:08:10Z

libs/cardano-ledger-core/src/Cardano/Ledger/PoolParams.hs


 -- | A stake pool.
-data PoolParams = PoolParams
+data StakePoolParams = StakePoolParams


all member should change such that instead of starting with pp , start with sp, yes?

teodanciu

Sorry, I didn't realize this was in Draft, so I went through it.
Most of my comments are about the following:
In PState, we already have psStakePoolParams and psFutureStakePoolParams, so no need to duplicate Stake .
And a few minor missed ones.

My take on your questions:

This PR made changes to many directories; thus, the corresponding CHANGELOG for all those sub-projects needs to be updated, yes?

Yes, but only for things that are visible from outside the packages - not internal names.

We should also change the module name from Cardano.Ledger.PoolParams to Cardano.Ledger.StakePoolParams yes?

I don't see why not. It would be good to do int a way that preserves git history

StatkePoolParams data members should start with sp, yes?

I would say even spp for prefix.

Will leave it to Alexey to approve, but otherwise looks good to me - barring the things above ^ .

teodanciu · 2025-08-05T11:27:50Z

libs/cardano-ledger-test/src/Test/Cardano/Ledger/Constrained/Conway/WitnessUniverse.hs

-  explainWit "poolparams :: (PoolParams c)" univ $
+  WitUniv era -> Specification StakePoolParams
+witStakePoolParamsSpec univ =
+  explainWit "poolparams :: (StakePoolParams c)" univ $


Missed one here:

Suggested change

explainWit "poolparams :: (StakePoolParams c)" univ $

explainWit "stakePoolParams :: (StakePoolParams c)" univ $

teodanciu · 2025-08-05T11:34:21Z

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

 -- | The state used by the POOL rule, which tracks stake pool information.
 data PState era = PState
-  { psStakePoolParams :: !(Map (KeyHash 'StakePool) PoolParams)
+  { psStakeStakePoolParams :: !(Map (KeyHash 'StakePool) StakePoolParams)


Don't think we need duplicated Stake here:

Suggested change

{ psStakeStakePoolParams :: !(Map (KeyHash 'StakePool) StakePoolParams)

{ psStakePoolParams :: !(Map (KeyHash 'StakePool) StakePoolParams)

teodanciu · 2025-08-05T11:35:07Z

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

+  { psStakeStakePoolParams :: !(Map (KeyHash 'StakePool) StakePoolParams)
  -- ^ The stake pool parameters.
-  , psFutureStakePoolParams :: !(Map (KeyHash 'StakePool) PoolParams)
+  , psFutureStakeStakePoolParams :: !(Map (KeyHash 'StakePool) StakePoolParams)


Same as above, stake is already in the name:

Suggested change

, psFutureStakeStakePoolParams :: !(Map (KeyHash 'StakePool) StakePoolParams)

, psFutureStakePoolParams :: !(Map (KeyHash 'StakePool) StakePoolParams)

teodanciu · 2025-08-05T11:36:04Z

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

+    [ "stakeStakePoolParams" .= psStakeStakePoolParams
+    , "futureStakeStakePoolParams" .= psFutureStakeStakePoolParams


The values in the keys should also not be duplicated:

Suggested change

[ "stakeStakePoolParams" .= psStakeStakePoolParams

, "futureStakeStakePoolParams" .= psFutureStakeStakePoolParams

[ "stakePoolParams" .= psStakePoolParams

, "futureStakePoolParams" .= psFutureStakePoolParams

teodanciu · 2025-08-05T11:37:32Z

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

+psStakeStakePoolParamsL :: Lens' (PState era) (Map (KeyHash 'StakePool) StakePoolParams)
+psStakeStakePoolParamsL = lens psStakeStakePoolParams (\ds u -> ds {psStakeStakePoolParams = u})


Extra Stake:

Suggested change

psStakeStakePoolParamsL :: Lens' (PState era) (Map (KeyHash 'StakePool) StakePoolParams)

psStakeStakePoolParamsL = lens psStakeStakePoolParams (\ds u -> ds {psStakeStakePoolParams = u})

psStakePoolParamsL :: Lens' (PState era) (Map (KeyHash 'StakePool) StakePoolParams)

psStakePoolParamsL = lens psStakePoolParams (\ds u -> ds {psStakePoolParams = u})

teodanciu · 2025-08-05T11:38:40Z

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

     in [ "stake" .= ssStake
        , "delegations" .= ssDelegations
-        , "poolParams" .= ssPoolParams
+        , "poolParams" .= ssStakePoolParams


Suggested change

, "poolParams" .= ssStakePoolParams

, "stakePoolParams" .= ssStakePoolParams

teodanciu · 2025-08-05T11:41:01Z

libs/cardano-ledger-api/src/Cardano/Ledger/Api/Tx/Body.hs

 --
 -- > let lookupRefund = lookupDepositDState (certDState dpState)
-- > let isRegPoolId = (`Map.member` psStakePoolParams (certPState dpState))
+-- > let isRegPoolId = (`Map.member` psStakeStakePoolParams (certPState dpState))


If we remove the duplicate Stake in PState this will be:

Suggested change

-- > let isRegPoolId = (`Map.member` psStakeStakePoolParams (certPState dpState))

-- > let isRegPoolId = (`Map.member` psStakePoolParams (certPState dpState))

teodanciu · 2025-08-05T11:42:55Z

eras/shelley/impl/src/Cardano/Ledger/Shelley/RewardProvenance.hs

  -- If this value is not at least as large as the 'pledgeRatioP',
  -- the stake pool will not earn any rewards for the given epoch.
-  , poolParamsP :: !PoolParams
+  , poolParamsP :: !StakePoolParams


Should we rename this too?

Suggested change

, poolParamsP :: !StakePoolParams

, stakePoolParamsP :: !StakePoolParams

teodanciu · 2025-08-05T11:44:16Z

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

+  psStakeStakePoolParamsL,
+  psFutureStakeStakePoolParamsL,


With the removing of the extra Stake in core these will be:

Suggested change

psStakeStakePoolParamsL,

psFutureStakeStakePoolParamsL,

psStakePoolParamsL,

psFutureStakePoolParamsL,

teodanciu · 2025-08-05T11:46:25Z

eras/conway/impl/testlib/Test/Cardano/Ledger/Conway/SPORatifySpec.hs

  , stakeNotVoted :: Coin
  , delegatees :: Map (Credential 'Staking) DRep
-  , poolParams :: Map (KeyHash 'StakePool) PoolParams
+  , poolParams :: Map (KeyHash 'StakePool) StakePoolParams


I know this is not so important, but we could change it too, for consistency:

Suggested change

, poolParams :: Map (KeyHash 'StakePool) StakePoolParams

, stakePoolParams :: Map (KeyHash 'StakePool) StakePoolParams

kayvank · 2025-08-05T13:00:23Z

Sorry, I didn't realize this was in Draft, so I went through it. Most of my comments are about the following: In PState, we already have psStakePoolParams and psFutureStakePoolParams, so no need to duplicate Stake . And a few minor missed ones.

My take on your questions:

This PR made changes to many directories; thus, the corresponding CHANGELOG for all those sub-projects needs to be updated, yes?

Yes, but only for things that are visible from outside the packages - not internal names.

We should also change the module name from Cardano.Ledger.PoolParams to Cardano.Ledger.StakePoolParams yes?

I don't see why not. It would be good to do int a way that preserves git history

StatkePoolParams data members should start with sp, yes?

I would say even spp for prefix.

Will leave it to Alexey to approve, but otherwise looks good to me - barring the things above ^ .

Thank you for taking the time to go through the PR. I'll fix the issues you pointed out later today, after hours :)

teodanciu · 2025-08-05T13:51:31Z

Also, I realize now that this PR is quite conflicting with: #5196, which is redefining PoolParams in a different module.
(maybe a be a good idea to coordinate on this ?) unless you're already aware of it. It's fine if it's just a matter of solving git conflicts, just pointing it out in case there's some misunderstanding.

kayvank · 2025-08-05T14:46:35Z

Also, I realize now that this PR is quite conflicting with: #5196, which is redefining PoolParams in a different module. (maybe a be a good idea to coordinate on this ?) unless you're already aware of it. It's fine if it's just a matter of solving git conflicts, just pointing it out in case there's some misunderstanding.

I was not aware of #5196. I should've gone through the pending PRs 1st. I did a preliminary rabae with #5156 , which resulted in 76 conflicts as of now.
Yes, it is best to coordinate. Thank you for pointing that out.
Perhaps it would be prudent to wait for #5196 to be merged into master, then rebase from master. What do you think?

tzw3 · 2025-08-05T15:05:00Z

one question: this file libs/cardano-ledger-conformance/src/Test/Cardano/Ledger/Conformance/Orphans.hs matches the pattern PoolParams , but it is skipped now, will/should it be renamed ?

kayvank · 2025-08-06T13:02:47Z

one question: this file libs/cardano-ledger-conformance/src/Test/Cardano/Ledger/Conformance/Orphans.hs matches the pattern PoolParams , but it is skipped now, will/should it be renamed ?

I think it might be best to wait for the earlier PR, #5196, to be completed before continuing with this work. For now, I have stopped working on this draft PR.

5191 Rename PoolParams

54ded1e

Rename PoolParams to StatekPoolParams, to avoid confusion with protocol parameters

kayvank force-pushed the 5191/RenamePoolParams branch from f9f17a3 to 54ded1e Compare August 5, 2025 00:48

kayvank mentioned this pull request Aug 5, 2025

Rename PoolParams to StakePoolParams #5191

Open

kayvank commented Aug 5, 2025

View reviewed changes

teodanciu reviewed Aug 5, 2025

View reviewed changes

	explainWit "poolparams :: (StakePoolParams c)" univ $
	explainWit "stakePoolParams :: (StakePoolParams c)" univ $

	{ psStakeStakePoolParams :: !(Map (KeyHash 'StakePool) StakePoolParams)
	{ psStakePoolParams :: !(Map (KeyHash 'StakePool) StakePoolParams)

	, psFutureStakeStakePoolParams :: !(Map (KeyHash 'StakePool) StakePoolParams)
	, psFutureStakePoolParams :: !(Map (KeyHash 'StakePool) StakePoolParams)

		[ "stakeStakePoolParams" .= psStakeStakePoolParams
		, "futureStakeStakePoolParams" .= psFutureStakeStakePoolParams

		psStakeStakePoolParamsL :: Lens' (PState era) (Map (KeyHash 'StakePool) StakePoolParams)
		psStakeStakePoolParamsL = lens psStakeStakePoolParams (\ds u -> ds {psStakeStakePoolParams = u})

	, "poolParams" .= ssStakePoolParams
	, "stakePoolParams" .= ssStakePoolParams

	-- > let isRegPoolId = (`Map.member` psStakeStakePoolParams (certPState dpState))
	-- > let isRegPoolId = (`Map.member` psStakePoolParams (certPState dpState))

	, poolParamsP :: !StakePoolParams
	, stakePoolParamsP :: !StakePoolParams

	, poolParams :: Map (KeyHash 'StakePool) StakePoolParams
	, stakePoolParams :: Map (KeyHash 'StakePool) StakePoolParams

5191 Rename PoolParams #5211

Are you sure you want to change the base?

5191 Rename PoolParams #5211

Uh oh!

Conversation

kayvank commented Aug 5, 2025

Description

Checklist

Uh oh!

kayvank commented Aug 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kayvank commented Aug 5, 2025

Uh oh!

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

Uh oh!

Uh oh!

kayvank commented Aug 5, 2025

Uh oh!

tzw3 commented Aug 5, 2025

Uh oh!

kayvank commented Aug 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

kayvank commented Aug 5, 2025 •

edited

Loading

teodanciu commented Aug 5, 2025 •

edited

Loading

kayvank commented Aug 6, 2025 •

edited

Loading