-
Notifications
You must be signed in to change notification settings - Fork 214
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 pool registry format updates #1078
Conversation
{ ticker = unsafeFromText "TICK" | ||
, homepage = "https://12345" | ||
, owner = unsafeFromText "ed25519_pk1z4vh8gva25w07x8574uujuveu8gz43fu6qfln3t4prcavrvcphjsk0pdqs" | ||
, name = "Pooley Mc-Poolface" |
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.
[question]
What do you think about testing some weird character sets in the name and/or description? Japanese and Chinese Kanjis, Cyrilic, Emojis and all sort of non-latin characters ?
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.
I think this is a good idea and we should do this through the QuickCheck generators.
My approach is to "ground" the tests using a fairly simple golden example, which is an actual zip file produced with the registry validator tool*.
Then attempt to test all the corner cases with randomly generated data.
(* however we need to wait for an update of the registry tool before we can download a proper example zip - the current one in this PR is synthetic).
3f782ae
to
184a2d8
Compare
9260022
to
413127b
Compare
50fb532
to
137f192
Compare
parseJSON = parseJSON >=> either (fail . show . ShowFmt) pure . fromText | ||
|
||
instance ToJSON PoolOwner where | ||
toJSON = toJSON . toText |
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.
[question]
It now seems a bit inconsistent that we enforce some quite strong validations on the PoolOwner
format, whereas we still do not validate other fields (like addresses) for instance. So, what is the rationale here 🤔 ? Shall we also enforce stricter validation for addresses there?
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.
The PoolOwner
is also going to be extracted from the registration certificate. It's no longer just a field of StakePoolMetadata
, so that's why.
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.
Got it 👍
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.
LGTM. But I wonder if we shouldn't be more consistent regarding the metadata validation?
bors r+ |
1078: Stake pool registry format updates r=rvl a=rvl Relates to #1076 # Overview This starts updating the types and tests for the new stake pool registry format, which is indexed by pool owner(s) rather than pool id. Co-authored-by: Rodney Lorrimar <[email protected]>
Build succeeded |
Relates to #1076
Overview
This starts updating the types and tests for the new stake pool registry format, which is indexed by pool owner(s) rather than pool id.