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

op-chain-ops/ecotone-scalar: prefer .FillBytes() to .Bytes() and copy #13472

Merged
merged 6 commits into from
Dec 19, 2024

Conversation

geoknee
Copy link
Contributor

@geoknee geoknee commented Dec 18, 2024

The latter can result in an error if scalar is less than 32 bytes in length (it gets padded incorrectly). A common value in use in the wild is scalar=684000 -- I've added a test case for this which fails with the existing implementation but passed with the fix.

After this is approved, we should cherry pick it on to #13412.


TODO

  • add some tests
  • make the util output JSON so the result is easier to parse

The latter can result in an error if the number is less than 32 bytes in length (it gets padded incorrectly).
@geoknee geoknee changed the title op-chain-ops/ecotone-scalar: prefer .FillBytes() to .Bytes() and copy op-chain-ops/ecotone-scalar: prefer .FillBytes() to .Bytes() and copy Dec 18, 2024
@geoknee geoknee requested a review from sebastianst December 18, 2024 22:11
Copy link

codecov bot commented Dec 18, 2024

Codecov Report

Attention: Patch coverage is 0% with 12 lines in your changes missing coverage. Please review.

Project coverage is 43.02%. Comparing base (b6cf8ee) to head (83a41d6).
Report is 1 commits behind head on develop.

Files with missing lines Patch % Lines
op-chain-ops/cmd/ecotone-scalar/main.go 0.00% 12 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #13472      +/-   ##
===========================================
- Coverage    47.59%   43.02%   -4.58%     
===========================================
  Files          943      775     -168     
  Lines        78827    69304    -9523     
  Branches       803        0     -803     
===========================================
- Hits         37515    29815    -7700     
+ Misses       38509    36955    -1554     
+ Partials      2803     2534     -269     
Flag Coverage Δ
cannon-go-tests-32 ?
cannon-go-tests-64 ?
contracts-bedrock-tests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
op-chain-ops/cmd/ecotone-scalar/main.go 0.00% <0.00%> (ø)

... and 177 files with indirect coverage changes

@geoknee geoknee marked this pull request as ready for review December 18, 2024 22:16
@geoknee geoknee requested review from a team as code owners December 18, 2024 22:16
Copy link
Member

@sebastianst sebastianst left a comment

Choose a reason for hiding this comment

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

is the tool supposed to support pre-Ecotone scalar encodings?

Copy link
Member

@sebastianst sebastianst left a comment

Choose a reason for hiding this comment

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

Approving, assuming that you'll still add that op mainnet test cast.

@geoknee geoknee enabled auto-merge December 19, 2024 17:05
@geoknee geoknee added this pull request to the merge queue Dec 19, 2024
Merged via the queue into develop with commit c377d51 Dec 19, 2024
40 checks passed
@geoknee geoknee deleted the gk/ecotone-scalars-fix branch December 19, 2024 17:16
geoknee added a commit that referenced this pull request Dec 20, 2024
…`copy` (#13472)

* prefer .FillBytes() to .Bytes() and copy

The latter can result in an error if the number is less than 32 bytes in length (it gets padded incorrectly).

* add tests

* output to JSON

* add test case for OPM

* tidy up

* use const in test
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.

2 participants