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

fix: adjust MaxGroupMemPrice #704

Merged
merged 1 commit into from Jun 24, 2020
Merged

fix: adjust MaxGroupMemPrice #704

merged 1 commit into from Jun 24, 2020

Conversation

troian
Copy link
Member

@troian troian commented Jun 24, 2020

if total amount of memory in group less then 1G
calculatePriceRange will always return range 0..1.

fixes #640 ?

@troian troian requested review from boz and Ropes as code owners June 24, 2020 17:02
@codecov
Copy link

codecov bot commented Jun 24, 2020

Codecov Report

Merging #704 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #704   +/-   ##
=======================================
  Coverage   30.91%   30.91%           
=======================================
  Files         144      144           
  Lines        6916     6916           
=======================================
  Hits         2138     2138           
  Misses       4670     4670           
  Partials      108      108           
Impacted Files Coverage Δ
validation/config.go 20.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8d399ba...8978bf4. Read the comment docs.


if defaultConfig.MaxGroupMemPrice < minDefaultMaxGroupMemPrice {
panic("invalid max group memory price")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

let's not do this. i agree with the sentiment but we should handle it more gracefully. probably need to refactor the way this validation works - it was added in a rush to make a (2018) testnet more user-friendly.

just skip it for now

Copy link
Member Author

Choose a reason for hiding this comment

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

fmt.Println instead of panic?

Copy link
Contributor

Choose a reason for hiding this comment

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

nah.

Copy link
Contributor

Choose a reason for hiding this comment

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

everything still works fine with a lower value, right? it's just that the prices are lower than we'd like.

Copy link
Member Author

Choose a reason for hiding this comment

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

kk. makes sense

if total amount of memory in group less then 1G
calculatePriceRange will always return range 0..1.

fixes #640 ?

Signed-off-by: Artur Troian <troian.ap@gmail.com>
Copy link
Contributor

@boz boz left a comment

Choose a reason for hiding this comment

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

🚢

@troian troian merged commit ed2ce2d into master Jun 24, 2020
@troian troian deleted the issue-640 branch June 24, 2020 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants