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

Return descriptive 503 when listing stake pools with uncomplete sync #2094

Conversation

hasufell
Copy link
Contributor

Wrt #1971

@hasufell hasufell force-pushed the hasufell/1971/fix-err-list-stakepools branch from 543a44b to 6baeef7 Compare August 31, 2020 15:04
Copy link
Member

@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.

Looks good so far. 👍

I've added some suggestions in the comments.

There are still some issues with formatting to address. (See comments.)

lib/core/src/Cardano/Wallet.hs Show resolved Hide resolved
lib/core/src/Cardano/Wallet.hs Outdated Show resolved Hide resolved
lib/shelley/src/Cardano/Wallet/Shelley/Pools.hs Outdated Show resolved Hide resolved
lib/shelley/src/Cardano/Wallet/Shelley/Pools.hs Outdated Show resolved Hide resolved
@piotr-iohk
Copy link
Contributor

@hasufell I have taken your branch (hasufell:hasufell/1971/fix-err-list-stakepools) for a little spin and I actually still see the same behavior as -> #1971 (comment) 🤔

Still see 500 and "Somenthing went wrong" on listing stake pools while wallet is syncing through Byron era. (Checked on testnet)

(btw, is there a reason why you are using your fork of cardano-wallet instead of using main repo?)

@hasufell
Copy link
Contributor Author

hasufell commented Sep 1, 2020

I can reproduce. This is the problem as described in #1971 (comment)

It's hard to say where this exception leaks to.

@hasufell hasufell force-pushed the hasufell/1971/fix-err-list-stakepools branch from 113fe0d to cf69b88 Compare September 1, 2020 16:07
@hasufell
Copy link
Contributor Author

hasufell commented Sep 1, 2020

@piotr-iohk should be fixed, please retry

Copy link
Member

@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.

Hi @hasufell

There are still some issues with coding style.

In a previous review, I made these requested changes, and they were marked as resolved, but they don't seem to be have been fixed (or if they were, perhaps the fixes were reverted by a subsequent push?):

Please could you address these issues?

As I mentioned before, I would love to be able to give you a tool that could automatically apply the correct format, but I don't have one. If you'd like to explore automated solutions such as brittany, I would not be opposed. But that requires a team discussion which is outside the scope of this PR.

Given that our team does not yet use an automated tool, team members generally make a best effort to make their code follow the standard. Could you do that? I don't think it's too hard to follow given that it's rather short, and only has a few rules. We provide an .editor file to make this easier, so the burden should not be too arduous.

As a reminder:

  • The purpose of having a written standard is to record things that the team has agreed on, so discussions about style can be avoided during PR reviews.
  • The benefit is that we can concentrate on more important issues during PR reviews, such as functionality and correctness.
  • Rules are added to the standard as the result of team discussions and voting. The standard is a consensus that we've worked out over time.

We allow ourselves the flexibility to occasionally break our own standard if there is good reason. But I don't think that this flexibility should be interpreted as licence to not follow the standard (on the basis that there is no automated tool), especially when the reviewer has gone to the effort of providing suggestions and feedback.

Thank-you.

lib/core/src/Cardano/Wallet/Api/Server.hs Outdated Show resolved Hide resolved
@hasufell hasufell force-pushed the hasufell/1971/fix-err-list-stakepools branch from 12281e3 to 51d9bf6 Compare September 2, 2020 07:28
@hasufell
Copy link
Contributor Author

hasufell commented Sep 2, 2020

Hi @jonathanknowles ,

thank you very much for your detailed explanation!

I believe there must be a misunderstanding though. I wasn't arguing against the coding standard in any of my PRs I believe, nor have I indicated that I don't intend to follow it. In fact I've spent a great deal of time configuring my editor, filing bug reports on various vim plugins and reading through stylish-haskell documentation in order to figure out if some of this can be automated (because we already use this tool). I'm using the .editorconfig since the first week, but it doesn't do much.

Maybe you're faster at pattern recognition and adapting to different coding styles. I personally am not and I've never worked with this coding style, so I'm afraid it will take more time before my PRs will satisfy your standards, unless this will be automated. Please bear with me.

Thanks for your time reviewing these points!

@piotr-iohk
Copy link
Contributor

@piotr-iohk should be fixed, please retry

LGTM.

GET http://localhost:8090/v2/stake-pools?stake=1000000000

The response was:
Code = 503,
Message = {"code":"past_horizon","message":"Tried to convert something that is past the horizon (due to uncertainty about the next hard fork). Wait for the node to finish syncing to the hard fork. Depending on the blockchain, this process can take an unknown amount of time."}

@KtorZ KtorZ dismissed jonathanknowles’s stale review September 2, 2020 14:03

code has been adjusted accordingly 👍

@hasufell
Copy link
Contributor Author

hasufell commented Sep 2, 2020

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Sep 2, 2020

This PR was included in a batch with a merge conflict, it will be automatically retried

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Sep 2, 2020

Merge conflict

@hasufell hasufell force-pushed the hasufell/1971/fix-err-list-stakepools branch 2 times, most recently from 6cd8b82 to 282e86b Compare September 2, 2020 18:05
@hasufell
Copy link
Contributor Author

hasufell commented Sep 2, 2020

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Sep 2, 2020

This PR was included in a batch with a merge conflict, it will be automatically retried

iohk-bors bot added a commit that referenced this pull request Sep 2, 2020
2094: Return descriptive 503 when listing stake pools with uncomplete sync r=hasufell a=hasufell

Wrt #1971

Co-authored-by: Julian Ospald <julian.ospald@iohk.io>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Sep 2, 2020

Build failed

@KtorZ
Copy link
Member

KtorZ commented Sep 3, 2020

bors r+

iohk-bors bot added a commit that referenced this pull request Sep 3, 2020
2094: Return descriptive 503 when listing stake pools with uncomplete sync r=KtorZ a=hasufell

Wrt #1971

Co-authored-by: Julian Ospald <julian.ospald@iohk.io>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Sep 3, 2020

Build failed

@hasufell hasufell force-pushed the hasufell/1971/fix-err-list-stakepools branch from 282e86b to 1b4692c Compare September 7, 2020 09:05
@hasufell
Copy link
Contributor Author

hasufell commented Sep 7, 2020

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Sep 7, 2020

Build succeeded:

@iohk-bors iohk-bors bot merged commit d7dc954 into cardano-foundation:master Sep 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants