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

fix(sha3)_: support hex string (#6216) #6221

Merged
merged 1 commit into from
Jan 3, 2025
Merged

Conversation

qfrank
Copy link
Contributor

@qfrank qfrank commented Dec 17, 2024

cherry pick from PR

@qfrank qfrank self-assigned this Dec 17, 2024
@qfrank qfrank requested a review from flexsurfer December 17, 2024 12:21
@status-im-auto
Copy link
Member

status-im-auto commented Dec 17, 2024

Jenkins Builds

Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ c70e1ad #1 2024-12-17 12:25:27 ~5 min macos 📦zip
✔️ c70e1ad #1 2024-12-17 12:25:47 ~5 min windows 📦zip
✔️ c70e1ad #1 2024-12-17 12:25:58 ~5 min ios 📦zip
✔️ c70e1ad #1 2024-12-17 12:26:22 ~6 min linux 📦zip
✔️ c70e1ad #1 2024-12-17 12:26:29 ~6 min android 📦aar
✔️ c70e1ad #1 2024-12-17 12:27:24 ~7 min tests-rpc 📄log
✔️ c70e1ad #1 2024-12-17 12:28:39 ~8 min macos 📦zip
✔️ c70e1ad #1 2024-12-17 12:51:54 ~31 min tests 📄log
✔️ e01e489 #2 2025-01-02 04:20:31 ~4 min macos 📦zip
✔️ e01e489 #2 2025-01-02 04:21:06 ~5 min ios 📦zip
✔️ e01e489 #2 2025-01-02 04:21:25 ~5 min linux 📦zip
✔️ e01e489 #2 2025-01-02 04:21:37 ~5 min android 📦aar
✔️ e01e489 #2 2025-01-02 04:21:45 ~5 min windows 📦zip
✔️ e01e489 #2 2025-01-02 04:22:18 ~6 min tests-rpc 📄log
✔️ e01e489 #2 2025-01-02 04:26:07 ~10 min macos 📦zip
✔️ e01e489 #2 2025-01-02 04:47:18 ~31 min tests 📄log

Copy link

codecov bot commented Dec 17, 2024

Codecov Report

Attention: Patch coverage is 70.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 61.44%. Comparing base (a4e36d4) to head (e01e489).
Report is 1 commits behind head on develop.

Files with missing lines Patch % Lines
abi-spec/utils.go 70.00% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #6221   +/-   ##
========================================
  Coverage    61.43%   61.44%           
========================================
  Files          834      834           
  Lines       109931   109940    +9     
========================================
+ Hits         67540    67554   +14     
- Misses       34494    34497    +3     
+ Partials      7897     7889    -8     
Flag Coverage Δ
functional 21.50% <0.00%> (+0.04%) ⬆️
unit 60.01% <70.00%> (-0.02%) ⬇️

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

Files with missing lines Coverage Δ
abi-spec/utils.go 77.11% <70.00%> (-0.87%) ⬇️

... and 37 files with indirect coverage changes

Copy link
Collaborator

@igor-sirotin igor-sirotin left a comment

Choose a reason for hiding this comment

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

As discussed in DM, I still believe that this function could be simplified to smth like this:

str = hex.RemovePrefix(str)
str = hex.DecodeString(str)
bytes = crypto.Keccak256([]byte(str))
return common.Bytes2Hex(bytes)

And we wouldn't have to duplicate the regex.

But this is a minor thing and @qfrank prefers to keep the function exactly as in web3.js, as it's what client was using before.

Anyway, this is not a big deal for me, so I'm approving.

@qfrank qfrank force-pushed the fix/sha3_cherry_pick branch from c70e1ad to e01e489 Compare January 2, 2025 04:15
@qfrank qfrank merged commit c84a35d into develop Jan 3, 2025
19 checks passed
@qfrank qfrank deleted the fix/sha3_cherry_pick branch January 3, 2025 00:06
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.

4 participants