feat: ecrecover copy solidity interface

[pandadefi]

What I did

Since vyper has uint8 now, ecrecover v should be using uint8.

How I did it

How to verify it

Commit message

Commit message for the final, squashed PR. (Optional, but reviewers will appreciate it! Please see our commit message style guide for what we would ideally like to see in a commit message.)

Description for the changelog

Cute Animal Picture

[codecov-commenter]

Codecov Report

Merging #3084 (fe1b414) into master (a021599) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #3084      +/-   ##
==========================================
- Coverage   88.34%   88.32%   -0.03%     
==========================================
  Files          98       98              
  Lines       11019    11024       +5     
  Branches     2604     2605       +1     
==========================================
+ Hits         9735     9737       +2     
- Misses        833      835       +2     
- Partials      451      452       +1     

Impacted Files	Coverage Δ
vyper/builtin_functions/functions.py	90.37% <100.00%> (-0.13%)	⬇️
vyper/ast/nodes.py	93.05% <0.00%> (-0.17%)	⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[fubuloubu]

This tracks how we do it in ape (which I've always had to convert over) and how Solidity also handles it with it's ecrecover, so I support this update

Probably needs to go and update any ecrecover tests, and also this is a breaking change that probably needs to wait for v0.4.0 to merge (what do you think @big-tech-sux?)

[charles-cooper]

well, v0.4.0 is the next release so we can probably start merging in breaking changes.

in any case(!) i think this change is small enough that we don't need to wait for v0.4.0 and we could merge it as part of the 0.3.x series.

[fubuloubu]

well, v0.4.0 is the next release so we can probably start merging in breaking changes.

in any case(!) i think this change is small enough that we don't need to wait for v0.4.0 and we could merge it as part of the 0.3.x series.

I disagree, this will definitely require at least a twitter shout out (above and beyond the release notes) telling people about this change so they can make updates to their code accordingly.

[pandadefi]

@charles-cooper @fubuloubu tests fixed, can move this forward.

[charles-cooper]

This seems fine. The only thing I am hesitant about is that conversions to uint8 (which this change will encourage the user to do) introduce extra opcodes to ensure the value is in bounds for uint8. This probably should not stop us from making the interface cleaner, but it's something to keep in mind.

[charles-cooper]

sorry based on offline conversation with @fubuloubu -- @pandadefi since point of this PR is just to make it syntactically simpler to call ecrecover with an existing uint8 (without having to convert()) but the ecrecover precompile just expects 32 bytes, it would introduce inefficiency for use cases where the user has a uint256 (since those are always checked to be in range for v anyways).

to address this, i think we should make ecrecover accept both kinds of input. in other words, v can be in (uint8, uint256) and r and s in (bytes32, uint256). this maintains backwards compatibility while supporting the intent of this PR. the only downside is that it makes the API more confusing, but there is no EVM-level reason to pick one of these types over another.

[pandadefi]

@charles-cooper can this be merged? It will not break existing function.

[charles-cooper]

@pandadefi thanks for your contribution!