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

chore[test]: update hexbytes version and tests #3904

Merged
merged 11 commits into from
Apr 4, 2024

Conversation

DanielSchiavini
Copy link
Contributor

@DanielSchiavini DanielSchiavini commented Apr 2, 2024

What I did

  • Updated the tests after they were broken by a HexBytes update.
  • eth-account got bumped from 0.11.0 to 0.12.0, which in turn bumped hexbytes to 1.2.0.
  • HexBytes 1.0.0 had a breaking change (Move HexBytes prepend of 0x from hex method to repr to not break hex of parent bytes class)

How I did it

  • Selected a more specific hexbytes version
  • Updated the tests

How to verify it

  • Tests should now pass.

Commit message

update some tests after they were broken by an upstream update.

`eth-account` got bumped from 0.11.0 to 0.12.0, which in turn bumped
`hexbytes` to 1.2.0. `hexbytes` 1.0.0 had a (long overdue) breaking
change, which was to change the `.hex()` method to not prepend "0x".

this commit updates our tests accordingly, and does a bit of refactoring
in some places to use `bytes` directly instead of `HexBytes` (including
that the `keccak` fixture is changed to use vyper.utils.keccak256
instead of the one provided by web3, which returns `HexBytes`). it also
cleans up some manual method id calculations with the utility functions.

Description for the changelog

Update hexbytes version

Cute Animal Picture

image

Copy link

socket-security bot commented Apr 2, 2024

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
pypi/[email protected] filesystem Transitive: environment, eval, network, shell, unsafe +33 53.3 MB carver, fselmo, kclowes, ...2 more

🚮 Removed packages: pypi/[email protected], pypi/[email protected]

View full report↗︎

@DanielSchiavini DanielSchiavini changed the title chore[tests]: Update hexbytes version and tests chore[test]: Update hexbytes version and tests Apr 2, 2024
setup.py Outdated Show resolved Hide resolved
@DanielSchiavini DanielSchiavini changed the title chore[test]: Update hexbytes version and tests chore[test]: update hexbytes version and tests Apr 2, 2024
@codecov-commenter
Copy link

codecov-commenter commented Apr 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.14%. Comparing base (45a225c) to head (3950238).

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3904      +/-   ##
==========================================
- Coverage   90.89%   89.14%   -1.76%     
==========================================
  Files          95       95              
  Lines       14388    14388              
  Branches     3186     3186              
==========================================
- Hits        13078    12826     -252     
- Misses        908     1112     +204     
- Partials      402      450      +48     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

setup.py Outdated Show resolved Hide resolved
@@ -1,6 +1,10 @@
from vyper.utils import hex_to_int


def test_sanity(keccak):
Copy link
Member

Choose a reason for hiding this comment

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

let's move this to tests/unit (maybe tests/unit/utils/test_keccak256.py)

@@ -1,6 +1,10 @@
from vyper.utils import hex_to_int


def test_sanity(keccak):
assert keccak(b"").hex() == "c5d2460186f7233c927e7db2dcc703c0e500b653ca82273b7bfad8045d85a470"
Copy link
Member

Choose a reason for hiding this comment

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

add comment - sha3 vs keccak

Suggested change
assert keccak(b"").hex() == "c5d2460186f7233c927e7db2dcc703c0e500b653ca82273b7bfad8045d85a470"
# ensure keccak is keccak256, not sha3
# https://ethereum.stackexchange.com/a/107985
assert keccak(b"").hex() == "c5d2460186f7233c927e7db2dcc703c0e500b653ca82273b7bfad8045d85a470"

@@ -41,7 +41,7 @@ def foo(bar: uint256) -> Bytes[36]:
contract = get_contract(code)

# 2fbebd38000000000000000000000000000000000000000000000000000000000000002a
method_id = keccak(text="foo(uint256)").hex()[2:10] # 2fbebd38
method_id = keccak(b"foo(uint256)").hex()[:8] # 2fbebd38
Copy link
Member

Choose a reason for hiding this comment

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

i guess we can use the method_id() util here to be consistent with the test_variable_assignment above

@charles-cooper charles-cooper enabled auto-merge (squash) April 4, 2024 14:30
@charles-cooper charles-cooper merged commit dc62e7a into vyperlang:master Apr 4, 2024
150 checks passed
@DanielSchiavini DanielSchiavini deleted the hexbytes branch April 4, 2024 18:10
electriclilies pushed a commit to electriclilies/vyper that referenced this pull request Apr 27, 2024
update some tests after they were broken by an upstream update.

`eth-account` got bumped from 0.11.0 to 0.12.0, which in turn bumped
`hexbytes` to 1.2.0. `hexbytes` 1.0.0 had a (long overdue) breaking
change, which was to change the `.hex()` method to not prepend "0x".

this commit updates our tests accordingly, and does a bit of refactoring
in some places to use `bytes` directly instead of `HexBytes` (including
that the `keccak` fixture is changed to use vyper.utils.keccak256
instead of the one provided by web3, which returns `HexBytes`). it also
cleans up some manual method id calculations with the utility functions.

---------

Co-authored-by: Charles Cooper <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants