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

Correct possible mistakes #1890

Merged
merged 2 commits into from Jul 14, 2019

Conversation

@yahiheb
Copy link
Collaborator

commented Jul 11, 2019

Please bear in mind I am not sure about these changes.
If they are correct shouldn't we use else if (feeTarget >= 1008) instead of else if (feeTarget == 1008)

@@ -579,7 +579,7 @@ private void SetFeesAndTexts()
var d = feeTarget / Constants.OneDayConfirmationTarget;
ConfirmationExpectedText = $"{d} {IfPlural(d, "day", "days")}";
}
else if (feeTarget == 10008)
else if (feeTarget == 1008)

This comment has been minimized.

Copy link
@benthecarman

benthecarman Jul 11, 2019

Collaborator

I think this is meant to be set to Constants.SevenDaysConfirmationTarget, good catch

This comment has been minimized.

Copy link
@lontivero

lontivero Jul 11, 2019

Collaborator

Given the displayed text is "two weeks", it should be (2 * Constants.SevenDaysConfirmationTarget) but, the max target allowed by bitcoin core is 1008 (one week) so, this has to be:

Suggested change
else if (feeTarget == 1008)
else if (feeTarget == Constants.SevenDaysConfirmationTarget)

feeTarget can never be greater that 1008 so, the condition == is correct. I don't know if the "two weeks` text is a joke or not but I would change it to one week.

Good catch!

This comment has been minimized.

Copy link
@yahiheb

yahiheb Jul 11, 2019

Author Collaborator

I used the const, and yes I think maybe we should say one week not two weeks™.
Notice also that there is after two weeks, I don't know why.

This comment has been minimized.

Copy link
@benthecarman

benthecarman Jul 11, 2019

Collaborator

the tm is there a joke/meme

This comment has been minimized.

Copy link
@MaxHillebrand

MaxHillebrand Jul 12, 2019

Collaborator

Hehe, @lontivero, are you again taking the fun out of the code?! 😄

@benthecarman
Copy link
Collaborator

left a comment

After changing to the constant, ACK

@lontivero
Copy link
Collaborator

left a comment

@nopara73 review this please.

@nopara73

This comment has been minimized.

Copy link
Collaborator

commented Jul 14, 2019

Great finds!

@nopara73 nopara73 merged commit fe4f626 into zkSNACKs:master Jul 14, 2019

1 check passed

CodeFactor No issues found.
Details

@yahiheb yahiheb deleted the yahiheb:not-sure-changes branch Jul 14, 2019

@nopara73 nopara73 referenced this pull request Jul 15, 2019
@nopara73

This comment has been minimized.

Copy link
Collaborator

commented Jul 25, 2019

We decided to give out an additional $100 bug bounty for this fix. Will settle it with the contribution game results.

@MaxHillebrand

This comment has been minimized.

Copy link
Collaborator

commented Jul 25, 2019

I'm sure this is a typo...
The bounty is 1 MILLION satoshis 😆

@yahiheb

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 25, 2019

Thank you so much, that is very generous of you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.