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.

Sign up for GitHub

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

Jump to bottom

Don't fetch nOpt another time #1982

Merged
merged 1 commit into from
Jul 29, 2020
Merged

Don't fetch nOpt another time #1982

merged 1 commit into from
Jul 29, 2020

Conversation

Anviking
Copy link
Collaborator

Issue Number

#1981

Overview

  • Re-use the existing nOpt instead of fetching it again

Comments

@Anviking
Don't fetch nOpt another time
dcce6c4 
nOpt is returned as part of the `stakeDistribution` function. It was
originally added for the purpose of calculating saturation.

Recently, along with logic for handling pools not-yet in the stake
distribution, this was added:
```
pp <- liftIO $ getProtocolParameters nl
```
also for the sake of retrieving nOpt (aka desiredNumberOfStakePools).

It is both slightly confusing to retrieve it from two different places,
and they are no longer guaranteed to be from the same point.
@Anviking Anviking self-assigned this Jul 29, 2020
Anviking
Anviking commented Jul 29, 2020
View reviewed changes
lib/shelley/src/Cardano/Wallet/Shelley/Pools.hs
@@ -272,11 +276,11 @@ data PoolDbData = PoolDbData
combineDbAndLsqData
:: forall m. (Monad m)
=> TimeInterpreter m
-> ProtocolParameters
-> Int -- ^ nOpt; desired number of pools
Copy link
Collaborator Author

@Anviking Anviking Jul 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Do we need a newtype DesiredNumberOfPools?

I'm not sure the need is really there yet, and making all the other packages compile again can be slightly annoying, so I didn't do it.

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, I do not see this newtype:

pawel@pure:~/Work/IOHK/cardano-wallet (master)$ ag DesiredNumberOfPools
lib/core/src/Cardano/Wallet/DB/Sqlite/TH.hs
168:    protocolParametersDesiredNumberOfPools  Word16          sql=desired_pool_number

lib/core/src/Cardano/Wallet/DB/Sqlite.hs
450:        addColumn conn True (DBField ProtocolParametersDesiredNumberOfPools) value

@Anviking Anviking requested a review from KtorZ July 29, 2020 13:44
paweljakubas
paweljakubas approved these changes Jul 29, 2020
View reviewed changes
Copy link
Contributor

@paweljakubas paweljakubas left a comment

Choose a reason for hiding this comment

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

one less rountrip. lgtm !

@Anviking
Copy link
Collaborator Author

bors r+

iohk-bors bot added a commit that referenced this pull request Jul 29, 2020
@iohk-bors @Anviking
Merge #1982
3675ef2 
1982: Don't fetch nOpt another time r=Anviking a=Anviking

# Issue Number

#1981 

# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->

- [x] Re-use the existing `nOpt` instead of fetching it again


# Comments

<!-- Additional comments or screenshots to attach if any -->

<!-- 
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Once created, link this PR to its corresponding ticket
 ✓ Assign the PR to a corresponding milestone
 ✓ Acknowledge any changes required to the Wiki
-->


Co-authored-by: Johannes Lund <johannes.lund@iohk.io>
@Anviking Anviking added the IMPROVEMENT Mark a PR as an improvement, for auto-generated CHANGELOG label Jul 29, 2020
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Jul 29, 2020 •
edited by Anviking
Loading

Build failed

Failures:

  src/Test/Integration/Scenario/API/Shelley/Addresses.hs:71:5:
  1) API Specifications BYRON_ADDRESS_LIST - Byron wallet on Shelley ep
       uncaught exception: ProcessHasExited
       ProcessHasExited "cluster didn't start correctly: [Left user error (Waited too long for: pool registration),Left user error (Waited too long for: pool registration),Left user error (Waited too long for: pool registration),Left user error (Waited too long for: pool registration)]" (ExitFailure 1)

  To rerun use: --match "/API Specifications/BYRON_ADDRESS_LIST - Byron wallet on Shelley ep/"

  2) afterAll-hook
       uncaught exception: ResultStatus
       Pending Nothing (Just "exception in beforeAll-hook (see previous failure)")

  To rerun use: --match "/afterAll-hook/"

Randomized with seed 1561007479

Finished in 172.4071 seconds
637 examples, 2 failures, 583 pending
builder for '/nix/store/rwdwqadkchg7xq7xalwnlpxykk216sjc-cardano-wallet-shelley-test-integration-2020.7.28-check.drv' failed with exit code 1

@Anviking
Copy link
Collaborator Author

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Jul 29, 2020

Build succeeded

@iohk-bors iohk-bors bot merged commit 4f51038 into master Jul 29, 2020
@iohk-bors iohk-bors bot deleted the anviking/1981/fix branch July 29, 2020 15:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@paweljakubas paweljakubas paweljakubas approved these changes

@KtorZ KtorZ Awaiting requested review from KtorZ

Assignees

@Anviking Anviking

Labels
IMPROVEMENT Mark a PR as an improvement, for auto-generated CHANGELOG
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@Anviking @paweljakubas