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

Merged
merged 1 commit into from
Dec 17, 2024
Merged

fix(sha3)_: support hex string #6216

merged 1 commit into from
Dec 17, 2024

Conversation

qfrank
Copy link
Contributor

@qfrank qfrank commented Dec 16, 2024

This PR adds a check to determine if a str starts with 0x.

Relate mobile issue.

This is what web3 implemented:

export const sha3 = (data: Bytes): string | undefined => {
    let updatedData: Uint8Array;

    if (typeof data === 'string') {
        if (data.startsWith('0x') && isHexStrict(data)) {
            updatedData = hexToBytes(data);
        } else {
            updatedData = utf8ToBytes(data);
        }
    } else {
        updatedData = data;
    }
    const hash = keccak256Wrapper(updatedData);

    // EIP-1052 if hash is equal to c5d2460186f7233c927e7db2dcc703c0e500b653ca82273b7bfad8045d85a470, keccak was given empty data
    return hash === SHA3_EMPTY_BYTES ? undefined : hash;
};

@qfrank qfrank self-assigned this Dec 16, 2024
@status-im-auto
Copy link
Member

status-im-auto commented Dec 16, 2024

Jenkins Builds

Click to see older builds (8)
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 5fef266 #1 2024-12-16 05:55:35 ~4 min macos 📦zip
✔️ 5fef266 #1 2024-12-16 05:55:43 ~4 min linux 📦zip
✖️ 5fef266 #1 2024-12-16 05:55:50 ~4 min tests 📄log
✔️ 5fef266 #1 2024-12-16 05:55:54 ~4 min ios 📦zip
✔️ 5fef266 #1 2024-12-16 05:56:35 ~5 min macos 📦zip
✔️ 5fef266 #1 2024-12-16 05:56:37 ~5 min windows 📦zip
✔️ 5fef266 #1 2024-12-16 05:56:59 ~5 min android 📦aar
✔️ 5fef266 #1 2024-12-16 05:58:06 ~6 min tests-rpc 📄log
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ eb6e86a #2 2024-12-16 06:18:25 ~4 min windows 📦zip
✔️ eb6e86a #2 2024-12-16 06:18:39 ~4 min macos 📦zip
✔️ eb6e86a #2 2024-12-16 06:18:44 ~4 min linux 📦zip
✔️ eb6e86a #2 2024-12-16 06:18:44 ~4 min ios 📦zip
✔️ eb6e86a #2 2024-12-16 06:19:22 ~5 min macos 📦zip
✔️ eb6e86a #2 2024-12-16 06:19:57 ~5 min android 📦aar
✔️ eb6e86a #2 2024-12-16 06:20:29 ~6 min tests-rpc 📄log
✖️ eb6e86a #2 2024-12-16 06:44:38 ~30 min tests 📄log
✔️ eb6e86a #3 2024-12-16 07:21:25 ~29 min tests 📄log
✔️ 3cf8b9b #3 2024-12-16 09:01:15 ~3 min windows 📦zip
✔️ 3cf8b9b #3 2024-12-16 09:01:40 ~4 min macos 📦zip
✔️ 3cf8b9b #3 2024-12-16 09:01:46 ~4 min linux 📦zip
✔️ 3cf8b9b #3 2024-12-16 09:02:05 ~4 min ios 📦zip
✔️ 3cf8b9b #3 2024-12-16 09:02:35 ~5 min macos 📦zip
✔️ 3cf8b9b #3 2024-12-16 09:03:28 ~6 min tests-rpc 📄log
✔️ 3cf8b9b #3 2024-12-16 09:03:31 ~6 min android 📦aar
✔️ 3cf8b9b #4 2024-12-16 09:26:59 ~29 min tests 📄log

Copy link

codecov bot commented Dec 16, 2024

Codecov Report

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

Project coverage is 60.92%. Comparing base (d07e61f) to head (3cf8b9b).
Report is 5 commits behind head on release/7.1.x.

Files with missing lines Patch % Lines
abi-spec/utils.go 70.00% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@                Coverage Diff                @@
##           release/7.1.x    #6216      +/-   ##
=================================================
- Coverage          60.93%   60.92%   -0.02%     
=================================================
  Files                832      832              
  Lines             109856   109865       +9     
=================================================
- Hits               66943    66930      -13     
- Misses             35066    35078      +12     
- Partials            7847     7857      +10     
Flag Coverage Δ
functional 13.90% <0.00%> (-0.01%) ⬇️
unit 60.03% <70.00%> (-0.03%) ⬇️

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 27 files with indirect coverage changes

@anastasiyaig
Copy link
Contributor

@saledjenic pls take a look

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.

@qfrank I'm ok to merge this for the release.

But for develop branch please fix the code duplication. We already have similar functions used in this file:

status-go/abi-spec/utils.go

Lines 149 to 150 in 3cf8b9b

address = removeHexPrefix(address)
addressHash := Sha3(strings.ToLower(address))

Perhaps Sha3 should do these 2 conversions.

abi-spec/utils_test.go Show resolved Hide resolved
@qfrank
Copy link
Contributor Author

qfrank commented Dec 17, 2024

@qfrank I'm ok to merge this for the release.

But for develop branch please fix the code duplication. We already have similar functions used in this file:

status-go/abi-spec/utils.go

Lines 149 to 150 in 3cf8b9b

address = removeHexPrefix(address)
addressHash := Sha3(strings.ToLower(address))

Perhaps Sha3 should do these 2 conversions.

There was a comment "implementation referenced from https://github.com/ChainSafe/web3.js/blob/edcd215bf657a4bba62fabaafd08e6e70040976e/packages/web3-utils/src/utils.js#L107" @igor-sirotin , its purpose was to align with web3

@igor-sirotin
Copy link
Collaborator

igor-sirotin commented Dec 17, 2024

its purpose was to align with web3

Sure, and I even found that code and checked it. But the fact that code int that library is polluted, doesn't mean we should do the same to our codebase 🙂

Less duplication -> Less chance to face a bug later

@igor-sirotin igor-sirotin merged commit b84be51 into release/7.1.x Dec 17, 2024
15 checks passed
@igor-sirotin igor-sirotin deleted the fix/sha3 branch December 17, 2024 11:30
qfrank added a commit that referenced this pull request Dec 17, 2024
igor-sirotin added a commit that referenced this pull request Dec 17, 2024
igor-sirotin added a commit that referenced this pull request Dec 17, 2024
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.

8 participants