Skip to content
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

Swagger adjustment: Supported pool id param is hex or bech32 #2093

Merged
merged 1 commit into from
Sep 1, 2020

Conversation

piotr-iohk
Copy link
Contributor

@piotr-iohk piotr-iohk commented Aug 31, 2020

Issue Number

#2023
#2025

Overview

  • 01d5d50
    Supported pool id param is hex or bech32

Comments

Before:

Screenshot from 2020-08-31 13-42-59

After:

Screenshot from 2020-08-31 16-16-37

@piotr-iohk piotr-iohk added the RESOLVING ISSUE Mark a PR as resolving issues, for auto-generated CHANGELOG label Aug 31, 2020
@piotr-iohk piotr-iohk requested a review from hasufell August 31, 2020 11:52
@piotr-iohk piotr-iohk self-assigned this Aug 31, 2020
maxLength: 56
minLength: 64
minLength: 56
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Empirically I see that pool ids (no matter if in hex or bech32 format) have consistently 56 char length 🤷‍♂️

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@KtorZ is that something we can guarantee? I'm not sure how deterministic bech32 is here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is right indeed, so we could leave it as a comment in the description instead.

Also, pool ids can be 64 chars ..... with jormungandr :s

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added a comment. Pls let me know it that's fine.

@@ -2226,6 +2224,9 @@ paths:
<p align="right">status: <strong>stable</strong></p>

Delegate all (current and future) addresses from the given wallet to the given stake pool.

<strong>Note:</strong> Stake pool id lenght can be undeterministic when encoded as bech32.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

length

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤦

@piotr-iohk piotr-iohk force-pushed the piotr/update_swagger_pool_id branch from 104c2b3 to 2017bc1 Compare August 31, 2020 14:15
@@ -2226,6 +2224,9 @@ paths:
<p align="right">status: <strong>stable</strong></p>

Delegate all (current and future) addresses from the given wallet to the given stake pool.

<strong>Note:</strong> Stake pool id length can be undeterministic when encoded as bech32.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can be --> is ?

Copy link
Contributor

@jonathanknowles jonathanknowles Sep 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure that "undeterministic" (which should probably be "nondeterministic") is the best way to word this.

Readers might infer that the bech32 encoding is nondeterministic in its length. Of course this is not true, as Bech32 encoding is completely deterministic.

Perhaps we could instead say something like:

Bech32-encoded stake pool identifiers can vary in length.

And perhaps then give some examples to illustrate the different lengths that are possible.

@piotr-iohk What do you think?

@jonathanknowles jonathanknowles self-requested a review September 1, 2020 04:48
Copy link
Contributor

@jonathanknowles jonathanknowles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@piotr-iohk

I'm not sure that "undeterministic" (which should probably read "nondeterministic") is the best way to word this.

Unfamiliar readers might infer that the bech32 encoding is nondeterministic in its length. (Of course as we know this isn't true, as Bech32 encoding is completely deterministic.)

Perhaps we could instead say something like:

Bech32-encoded stake pool identifiers can vary in length.

And perhaps then give some examples to illustrate the different lengths that are possible.

What do you think?

@@ -2226,6 +2224,9 @@ paths:
<p align="right">status: <strong>stable</strong></p>

Delegate all (current and future) addresses from the given wallet to the given stake pool.

<strong>Note:</strong> Stake pool id length can be undeterministic when encoded as bech32.
Copy link
Contributor

@jonathanknowles jonathanknowles Sep 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure that "undeterministic" (which should probably be "nondeterministic") is the best way to word this.

Readers might infer that the bech32 encoding is nondeterministic in its length. Of course this is not true, as Bech32 encoding is completely deterministic.

Perhaps we could instead say something like:

Bech32-encoded stake pool identifiers can vary in length.

And perhaps then give some examples to illustrate the different lengths that are possible.

@piotr-iohk What do you think?

@piotr-iohk piotr-iohk force-pushed the piotr/update_swagger_pool_id branch from 2017bc1 to 985a129 Compare September 1, 2020 06:11
@piotr-iohk
Copy link
Contributor Author

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Sep 1, 2020

Build succeeded

@iohk-bors iohk-bors bot merged commit 406c35f into master Sep 1, 2020
@iohk-bors iohk-bors bot deleted the piotr/update_swagger_pool_id branch September 1, 2020 13:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RESOLVING ISSUE Mark a PR as resolving issues, for auto-generated CHANGELOG
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants