< 8000 bdi class="js-issue-title markdown-title">Dynamic struct encoding #135
Conversation
Codecov Report
@@ Coverage Diff @@
## main #135 +/- ##
===========================================
- Coverage 99.74% 81.20% -18.55%
===========================================
Files 67 67
Lines 3899 4059 +160
===========================================
- Hits 3889 3296 -593
- Misses 10 763 +753
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
hey @q9f could you help me review this please |
| @@ -98,15 +109,13 @@ def ==(another_type) | |||
| def size | |||
| s = nil | |||
| if dimensions.empty? | |||
| unless ["string", "bytes"].include?(base_type) and sub_type.empty? | |||
| if !(["string", "bytes", "tuple"].include?(base_type) and sub_type.empty?) | |||
There was a problem hiding this comment.
nit: any reason you changed unless to !if?
There was a problem hiding this comment.
- https://stackoverflow.com/a/19216609
- https://stackoverflow.com/questions/41196492/ruby-unless-bug-with-multiple-conditions
- https://signalvnoise.com/posts/2699-making-sense-with-rubys-unless
- The
andkeyword having lower operator precedence than&&definitely didn't help with the readability
Some good rules of thumb:
- Prefer
ifoverunless90% of the time. - Prefer
ifoverunless100% of the time if there are multiple conditions. - Never use
andororornot.
| @@ -138,6 +161,7 @@ def validate_base_type(base_type, sub_type) | |||
|
|
|||
| # bytes can be no longer than 32 bytes | |||
| raise ParseError, "Maximum 32 bytes for fixed-length string or bytes" unless sub_type.empty? || sub_type.to_i <= 32 | |||
| when "tuple" | |||
There was a problem hiding this comment.
what about:
raise ParseError if (sub_type.dimensions.size != 1 || sub_type.dimensions.first&.zero?) && !sub_type.is_dynamic?Just taking a stab at it
| elsif type.is_dynamic? | ||
| raise EncodingError, "Argument must be an Array" unless arg.instance_of? Array | ||
|
|
||
| elsif type.base_type == "tuple" && type.dimensions.size == 1 && type.dimensions[0] != 0 |
There was a problem hiding this comment.
If type.dimensions[0] is nil, this would evaluate to true. Just wanted to make sure that was ok.
| # encodes dynamic-sized arrays | ||
| head, tail = "", "" | ||
| head += encode_type Type.size_type, arg.size | ||
| head += encode_type(Type.size_type, arg.size) |
| @@ -123,6 +134,22 @@ def encode_type(type, arg) | |||
| end | |||
| end | |||
|
|
|||
| def encode_struct_offsets(type, arg) | |||
There was a problem hiding this comment.
Can you add documentation here? I can't tell what this method is doing :(
I don't understand why there's so much class-level code, rather than using OOP and letting Ruby shine. But I guess that's a question for @q9f ❤️ ❤️
I would love to help get an idiomatic Ruby version of this library ready for a 1.0.0 release
There was a problem hiding this comment.
This class is almost as old as Ruby, I haven't written it. I rewrote everything except for the ABI parser and RLP encoder. Are you volunteering? :)
|
I'll be accepting this as is and resolve your comments on a new branch @mculp (I don't have write access here.) |
Co-authored-by: Matt Culpepper <mculp@users.noreply.github.com>
Co-authored-by: Matt Culpepper <mculp@users.noreply.github.com>
Co-authored-by: Matt Culpepper <mculp@users.noreply.github.com>
Co-authored-by: Matt Culpepper <mculp@users.noreply.github.com>
Co-authored-by: Matt Culpepper <mculp@users.noreply.github.com>
Co-authored-by: Matt Culpepper <mculp@users.noreply.github.com>
Co-authored-by: Matt Culpepper <mculp@users.noreply.github.com>
Co-authored-by: Matt Culpepper <mculp@users.noreply.github.com>
Co-authored-by: Matt Culpepper <mculp@users.noreply.github.com>
Co-authored-by: Matt Culpepper <mculp@users.noreply.github.com>
Co-authored-by: Matt Culpepper <mculp@users.noreply.github.com>
Co-authored-by: Matt Culpepper <mculp@users.noreply.github.com>
Co-authored-by: Matt Culpepper <mculp@users.noreply.github.com>
| @@ -73,6 +83,8 @@ def parse(type) | |||
| @base_type = base_type | |||
| @sub_type = sub_type | |||
| @dimensions = dims.map { |x| x[1...-1].to_i } | |||
| @components = components.map { |component| Eth::Abi::Type.parse(component["type"], component.dig("components"), component.dig("name")) } if components.any? | |||
There was a problem hiding this comment.
components.any? might fail if components = nil
| raise EncodingError, "Expecting #{type.components.size} elements: #{arg}" unless arg.size == type.components.size | ||
|
|
||
| static_size = 0 | ||
| type.components.each do |component| |
There was a problem hiding this comment.
Should be type.components.each_with_index do |component, i|
| component_type = type.components[i] | ||
| if component_type.is_dynamic? | ||
| offsets_and_static_values << encode_type(Type.size_type, dynamic_offset) | ||
| dynamic_value << encode_type(component_type, arg.is_a?(Array) ? arg[i] : arg[component_type.name]) |
* add test case * eth/abi: support dynamic struct encoding * remove unused function * typo * uncomment spec * cleanup spec * test tuple2 * add tuple2.sol * Update lib/eth/contract/function_input.rb * Update lib/eth/client.rb * Update lib/eth/contract/function.rb * Update lib/eth/abi/type.rb * Update lib/eth/abi.rb * Update lib/eth/abi.rb * Update lib/eth/abi/type.rb * Update lib/eth/abi/type.rb * Update lib/eth/abi/type.rb * Update lib/eth/abi.rb * Update lib/eth/abi.rb * Update lib/eth/abi.rb * Update lib/eth/abi/type.rb * eth/abi: fix tests for dynamic struct encoding * eth/abi: fix documentation and formatting Co-authored-by: Peter Chung <peter.chung@fazzfinancial.com>
adding abi encoder for the following data types