Update registration ownership info in monitorStakePools #1154
Conversation
1131: Include registration certificates in Cardano.Pool.Metrics.Block r=rvl a=rvl # Issue Number Relates to #1090. # Overview - Adds registration certificates to `Cardano.Pool.Metrics.Block`. # Comments - Testing will be done in #1154. Co-authored-by: Rodney Lorrimar <[email protected]>
…ain parameters This inconsistency was unfortunate. The static blockchain parameter function is so-to-speak partial, and we should ideally have different types for the genesis block and for the other blocks, but that something I'd rather do in a recovery week
KtorZ
force-pushed
the
rvl/1092/pool-registrations-db
branch
from
2354a1f
to
4691945
Compare
KtorZ
changed the base branch from
rvl/1090/apply-block-pool-registrations
to
master
There was a problem hiding this comment.
@rvl I've completed the unit test but will leave the review to you.
In the end, I am not sure it's buying us much more than what the db properties already buy us in terms of testing..
Still, I carried on because this test setup could open some nice possibilities to also test other stuff like the metrics.
|
bors try |
tryBuild succeeded |
rvl
force-pushed
the
rvl/1092/pool-registrations-db
branch
from
4d61438
to
9851083
Compare
|
Thanks @KtorZ.
I thought the same thing. There is only a small amount of logic in I have added an extra assertion for to the test case that stake pool registration certificates are logged. |
| <> pretty registrations | ||
| liftIO $ forM_ registrations $ \registration -> | ||
| logInfo tr $ "Discovered stake pool registration: " | ||
| <> pretty registration |
There was a problem hiding this comment.
I reviewed the following changes:
LGTM!
Though it'd be great to have a named constant for the magic number 42. (See: 9851083#diff-baca5dbe242634249ae8122255a26866R317)
| @@ -288,6 +312,9 @@ prop_trackRegistrations test = monadicIO $ do | |||
| pure (0, mempty) | |||
| , networkTip = | |||
| pure header0 | |||
| , staticBlockchainParameters = | |||
| (block0, mockBlockchainParameters | |||
| { getEpochStability = Quantity 42 }) | |||
There was a problem hiding this comment.
Question: where does the number 42 come from, and does it have a special meaning?
Even if it's just an arbitrary number, perhaps adding a named constant will help future readers to know that it's just an arbitrary number? (For example, arbitraryEpochStability or mockEpochStability.)
rvl
force-pushed
the
rvl/1092/pool-registrations-db
branch
from
9851083
to
b941225
Compare
|
bors merge |
|
bors r+ |
1154: Update registration ownership info in monitorStakePools r=rvl a=rvl Relates to #1092. # Overview This modifies the stake pool network follower loop so that pool registrations are tracked in the database. Tested with a unit test that mocks the network layer. Co-authored-by: Rodney Lorrimar <[email protected]> Co-authored-by: KtorZ <[email protected]>
Build succeeded |
Relates to #1092.
Overview
This modifies the stake pool network follower loop so that pool registrations are tracked in the database.
Tested with a unit test that mocks the network layer.