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

Abi.decoder handles arrays of string and bytes #207

Merged
merged 3 commits into from
Jan 17, 2023
Merged

Abi.decoder handles arrays of string and bytes #207

merged 3 commits into from
Jan 17, 2023

Conversation

randoum
Copy link
Contributor

@randoum randoum commented Jan 12, 2023

Before

> enc = Eth::Abi.encode ["string[]"], [["This is", "a test"]]
=> "\x00\x00\...
> Eth::Abi.decode ["string[]"], enc
.../gems/eth-0.5.10/lib/eth/abi.rb:232:in `decode_type': Wrong data size for string/bytes object (Eth::Abi::DecodingError)

After

> enc = Eth::Abi.encode ["string[]"], [["This is", "a test"]]
=> "\x00\x00\...
> Eth::Abi.decode ["string[]"], enc
=> [["This is", "a test"]]

(1..l).map do |i|
pointer = Util.deserialize_big_endian_to_int arg[i * 32, 32] # Pointer to the size of the array's element
data_l = Util.deserialize_big_endian_to_int arg[32 + pointer, 32] # length of the element
type(Type.parse(type.base_type), arg[pointer + 32, Util.ceil32(data_l) + 32])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This: Type.parse(type.base_type) feels kind of hacky. Please comment if it's acceptable

Copy link
Owner

Choose a reason for hiding this comment

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

That's ok. It might have a negative hit on the performance but nobody uses Ruby for performance anyways :)

@q9f
Copy link
Owner

q9f commented Jan 12, 2023

Thanks. You can ignore the failing tests.

I will take a look at this in a bit.

@randoum
Copy link
Contributor Author

randoum commented Jan 12, 2023

Thanks.

I've also fixed the decoding of fixed size arrays

@@ -48,7 +61,7 @@ def type(type, arg)
# decoded dynamic-sized arrays
(0...l).map { |i| type(nested_sub, arg[32 + nested_sub.size * i, nested_sub.size]) }
elsif !type.dimensions.empty?
l = type.dimensions.last[0]
l = type.dimensions.first
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be honest, I'm not sure what I did here 😅 but now all the tests pass, and it fixes the problem with static-size array decoding. I tried to check in git history why this was introduced but could not find anything understandable for me. Please share your feedback

Copy link
Owner

Choose a reason for hiding this comment

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

@codecov-commenter
Copy link

codecov-commenter commented Jan 16, 2023

Codecov Report

Merging #207 (aa5b790) into main (5f37cd4) will decrease coverage by 0.25%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #207      +/-   ##
==========================================
- Coverage   99.77%   99.51%   -0.26%     
==========================================
  Files          77       77              
  Lines        4361     4367       +6     
==========================================
- Hits         4351     4346       -5     
- Misses         10       21      +11     
Impacted Files Coverage Δ
spec/eth/abi_spec.rb 100.00% <ø> (ø)
lib/eth/abi/decoder.rb 100.00% <100.00%> (ø)
spec/eth/ens/resolver_spec.rb 83.78% <0.00%> (-16.22%) ⬇️
lib/eth/ens/resolver.rb 85.29% <0.00%> (-14.71%) ⬇️

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

@q9f q9f added the bug Something isn't working label Jan 16, 2023
Copy link
Owner

@q9f q9f left a comment

Choose a reason for hiding this comment

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

Looks good to me and tests locally run through.

Could you run rufo . and check in the produced changes?

@randoum
Copy link
Contributor Author

randoum commented Jan 17, 2023

Done

@q9f q9f merged commit d9034a5 into q9f:main Jan 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants