Skip to content

Fix getmininginfo unmarshal error #1644

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
Merged
merged 1 commit into from
Oct 26, 2020
Merged

Fix getmininginfo unmarshal error #1644

merged 1 commit into from
Oct 26, 2020

Conversation

@DavidMazary
Copy link
Contributor

@DavidMazary DavidMazary commented Oct 4, 2020

Since JSON doesn't distinguish between ints and floats in its number type, Go will fail to decode valid integers in scientific notation as noted in #1458

  1. Added a unit test for this condition that fails before the fix and passes after
  2. Verified that output values and format were unchanged when running btcctl getmininginfo

@coveralls
Copy link

coveralls commented Oct 4, 2020 •
edited
Loading

Pull Request Test Coverage Report for Build 287285588

Warning: This coverage report may be inaccurate.

We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
To ensure accuracy in future PRs, please see these guidelines.
A quick fix for this PR: rebase it; your next report should be accurate.

  • 0 of 2 (0.0%) changed or added relevant lines in 1 file are covered.
  • 5 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.01%) to 53.578%
Changes Missing Coverage Covered Lines Changed/Added Lines %
rpcserver.go 0 2 0.0%
Files with Coverage Reduction New Missed Lines %
peer/peer.go 2 76.1%
btcec/signature.go 3 92.48%
Totals Coverage Status
Change from base Build 285019835: 0.01%
Covered Lines: 20739
Relevant Lines: 38708
💛 - Coveralls

@jakesylvestre
Copy link
Collaborator

jakesylvestre commented Oct 4, 2020

can you rebase into a single commit?

onyb
onyb approved these changes Oct 8, 2020
View reviewed changes
Copy link
Contributor

@onyb onyb left a comment

Choose a reason for hiding this comment

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

Congratulations on your first PR to btcd! 🎉

@onyb
Copy link
Contributor

onyb commented Oct 8, 2020

@DavidMazary Just needs a rebase to fix the merge conflicts.

@onyb onyb added this to the 0.22.0 milestone Oct 12, 2020
@jcvernaleo
Copy link
Member

jcvernaleo commented Oct 12, 2020

Just fix the conflict and this is good to go in!

jcvernaleo
jcvernaleo approved these changes Oct 19, 2020
View reviewed changes
Copy link
Member

@jcvernaleo jcvernaleo left a comment

Choose a reason for hiding this comment

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

OK

jcvernaleo
jcvernaleo requested changes Oct 19, 2020
View reviewed changes
Copy link
Member

@jcvernaleo jcvernaleo left a comment

Choose a reason for hiding this comment

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

Fix merge conflicts and then this is good to go.

Thanks!

@DavidMazary
Copy link
Contributor Author

DavidMazary commented Oct 21, 2020

Fixed merge conflicts!

jcvernaleo
jcvernaleo approved these changes Oct 26, 2020
View reviewed changes
Copy link
Member

@jcvernaleo jcvernaleo left a comment

Choose a reason for hiding this comment

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

OK

@jcvernaleo jcvernaleo merged commit 6adfc07 into btcsuite:master Oct 26, 2020
This was referenced Apr 13, 2021
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jcvernaleo jcvernaleo jcvernaleo approved these changes

+1 more reviewer

@onyb onyb onyb approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

0.22.0

Development

Successfully merging this pull request may close these issues.

5 participants

@DavidMazary @coveralls @jakesylvestre @onyb @jcvernaleo