-
Notifications
You must be signed in to change notification settings - Fork 213
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
listStakePools - Include metadata in API responses #1159
Conversation
getRegistryZipUrl :: IO String | ||
getRegistryZipUrl = fromMaybe cardanoFoundationRegistryZip <$> lookupEnv var | ||
where | ||
var = "CARDANO_WALLET_STAKE_POOL_REGISTRY_URL" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
lib/core/src/Cardano/Pool/Metrics.hs
Outdated
newStakePoolLayer db@DBLayer{..} nl tr = StakePoolLayer | ||
{ listStakePools = do | ||
lift $ logTrace tr MsgListStakePoolsBegin | ||
stakePools <- rankKnownPools |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[remark]
You have used to forbidden word (i.e. rank
) 😱
It makes things clearer though 👍
82cb177
to
c6b2edc
Compare
…tegration test setup
c6b2edc
to
4de52ca
Compare
(Map.fromList [(pid, [owner])]) | ||
[(owner, Just md)] | ||
|
||
res `shouldBe` [(MsgMetadataUsing pid owner md, Just md)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
(Map.fromList [(PoolId "1", [PoolOwner "a"])]) | ||
[(PoolOwner "a", Nothing)] | ||
|
||
res `shouldBe` [(MsgMetadataMissing (PoolId "1"), Nothing)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
md `shouldSatisfy` sameStakePoolMetadata mdb | ||
_ -> fail $ "wrong log message " ++ show msg | ||
|
||
fmap (sameStakePoolMetadata mda) res `shouldBe` Just True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
…ion with other tests
…treated correctly
…for zipping correctly!
02fa548
to
baddeec
Compare
@@ -170,6 +183,30 @@ spec = do | |||
expectResponseCode @IO HTTP.status405 r | |||
expectErrorMessage errMsg405 r | |||
|
|||
it "STAKE_POOLS_LIST_04 - Discovers new pools when they are registered" $ \ctx -> do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rvl I added a rather extensive integration test. The setup behind registerStakePool
is a bit complex because we have to do everything with jcli
and use accounts to pay for the registration certificate! I got to the end of it and.. nicely caught a small bug in the "zipping logic" from the listStakePools
endpoint (cf my last commit).
Please have another round of review / fixes and let's get this merged!
Some outstanding points remaining after that:
- Maybe do some caching? I expect this endpoint to be quite heavily hammered by clients like Daedalus. The registry won't change very often. Renewing a cache every hour or so should also mitigate the concern.
- Currently the handler for
joinStakePool
relies onlistStakePool
to get a list of known stake pool... that's kinda lame becauselistStakePool
is a very heavy operation and, all stake pools ids can be known by simply making a database call. So we should fix that. - Apparent performance could be wrong for brief times (because of multiple sources of truth) #1158
bors try |
tryBuild failed |
https://github.com/input-output-hk/testnet-stake-pool-registry/archive/master.zip Current revision: c6d196f630b28869bffb9e3af453a9c65be2fa7c
bors r+ |
1159: listStakePools - Include metadata in API responses r=rvl a=rvl Relates to #1065. # Overview - Modified Api.Server.listPools and StakePoolLayer listStakePools to include metadata. - Made it possible to use an environment variable for the registry url. - Handles the case where different stake pool owners register different metadata by logging and then ignoring all inconsistent data. # Comments - This is a draft. Co-authored-by: Rodney Lorrimar <rodney.lorrimar@iohk.io> Co-authored-by: KtorZ <matthias.benkort@gmail.com>
Build succeeded |
Relates to #1065.
Overview
Comments