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

vague error when the private key passed to the addr cheatcode is greater than the secp256k1 curve order #3031

Closed
2 tasks done
odyslam opened this issue Aug 31, 2022 · 3 comments · Fixed by #3041
Closed
2 tasks done
Labels
T-bug Type: bug

Comments

@odyslam
Copy link
Contributor

odyslam commented Aug 31, 2022

Component

Forge

Have you ensured that all of these are up to date?

  • Foundry
  • Foundryup

What version of Foundry are you on?

forge 0.2.0 (20e7386 2022-08-31T00:09:48.876171Z)

What command(s) is the bug in?

forge test

Operating System

macOS (Apple Silicon)

Describe the bug

Same as #2158

Perhaps it would be nice to bake something in forge or forge-std so the user doesn't have to do

vm.assume( privKey != 0 && privKey < 115792089237316195423570985008687907852837564279074904382605163141518161494337)
``` all the time. I imagine that it's a fairly common use case
@odyslam odyslam added the T-bug Type: bug label Aug 31, 2022
@mds1
Copy link
Collaborator

mds1 commented Aug 31, 2022

What error message do you see? It should be [FAIL. Reason: Private key must be less than 115792089237316195423570985008687907852837564279074904382605163141518161494337 (the secp256k1 curve order). which I think is pretty descriptive. More info here: #2164 (comment)

Instead of assume you can also use uint248 as the input type for the private key: this obviously shrinks the range of private keys you'll get since type(uint248).max < curve order, but it should be good enough for most use cases. Though that'll still revert on zero, so using bound is another option

@odyslam
Copy link
Contributor Author

odyslam commented Aug 31, 2022

I see the error described in #2158

Interesting. I assumed it's best practice to use assume over bound.

@mds1
Copy link
Collaborator

mds1 commented Aug 31, 2022

Ok I see:

  • Passing in the curve order exactly results in signature error
  • Passing anything 1 greater than curve order results in the expected error message

Should be an easy fix, I'll get a PR in.

Re assume vs. bound, the assume condition here is broad enough that assume should be fine. For "narrow" conditions, you might get a lot of rejects and bound of course would have zero rejects so would result in a faster test suite. "Broad" and "tight" are grey, there's no hard rule here and definitely some personal preference involved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-bug Type: bug
Projects
No open projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants