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

use current data when calculating nonmyopic rewards #1724

Merged

Conversation

JaredCorduan
Copy link
Contributor

No description provided.

@JaredCorduan JaredCorduan merged commit 1402660 into master Jul 28, 2020
@iohk-bors iohk-bors bot deleted the use-current-pool-data-for-nonmyopic-information branch July 28, 2020 01:32
Copy link
Contributor

@kevinhammond kevinhammond left a comment

Choose a reason for hiding this comment

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

I have reviewed this code as a possible patch for 1.18.0, so I am looking at this from the perspective of what possible impacts there could be on a stable version (rather than the usual "does the code do what it says on the tin").

Summary: This is LOW RISK for deployment.

The code doesn't impact core functionality, so would be low risk as a patch. It is not least risk (e.g. changing a constant or calculation).

Lines 33-38 add new imports

Line 57: the Crypto class is probably extraneous (shouldn't change any existing functionality)

Lines 81 and 86 are new calls, which could add execution costs [they are apparently maps of some kind so there is potential for this to be significant] and perhaps add some risk of non-termination/failure.

The other main changes are essentially to lookup operations (EpochBoundary/EB). I haven't traced these down but they should have a low probability of changing functionality or performance characteristics.

Any performance changes should be localised. The code is only executed on demand when the wallet queries it, so wallet testing should identify any issues here.

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

3 participants