Dynamic struct encoding by peter-chung-xfers · Pull Request #135 · q9f/eth.rb

peter-chung-xfers · 2022-07-10T18:43:19Z

adding abi encoder for the following data types

dynamic struct
static struct
dynamic array of dynamic struct
dynamic array of static struct
static array of dynamic struct
static array of static struct

codecov-commenter · 2022-07-10T18:46:44Z

Codecov Report

Merging #135 (1699cac) into main (6e1a491) will decrease coverage by 18.54%.
The diff coverage is 24.50%.

❗ Current head 1699cac differs from pull request most recent head c567dc1. Consider uploading reports for the commit c567dc1 to get more accurate results

@@             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

Impacted Files	Coverage Δ
lib/eth/client.rb	`55.00% <0.00%> (-45.00%)`	⬇️
lib/eth/contract/function.rb	`75.00% <0.00%> (-25.00%)`	⬇️
lib/eth/abi.rb	`15.55% <7.14%> (-84.45%)`	⬇️
spec/eth/contract_spec.rb	`53.33% <13.33%> (-46.67%)`	⬇️
spec/eth/abi/type_spec.rb	`45.23% <17.64%> (-54.77%)`	⬇️
lib/eth/abi/type.rb	`72.22% <36.36%> (-27.78%)`	⬇️
spec/eth/abi_spec.rb	`34.50% <50.00%> (-65.50%)`	⬇️
lib/eth/contract/function_input.rb	`72.72% <66.66%> (-27.28%)`	⬇️
spec/eth/contract/function_spec.rb	`57.50% <85.71%> (-42.50%)`	⬇️
... and 29 more

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

peter-chung-xfers · 2022-07-18T05:20:29Z

hey @q9f could you help me review this please

q9f

Thank you very much, this is very well done!

I only left two minor comments.

q9f · 2022-09-26T15:33:47Z

lib/eth/abi/type.rb

        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?)


nit: any reason you changed unless to !if?

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 and keyword having lower operator precedence than && definitely didn't help with the readability

Some good rules of thumb:

Prefer if over unless 90% of the time.

Prefer if over unless 100% of the time if there are multiple conditions.

Never use and or or or not.

q9f · 2022-09-26T15:35:32Z

lib/eth/abi/type.rb


          # 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"


any idea how to validate this?

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

lib/eth/abi/type.rb

lib/eth/abi.rb

mculp · 2022-09-27T04:33:28Z

lib/eth/abi.rb

-      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


If type.dimensions[0] is nil, this would evaluate to true. Just wanted to make sure that was ok.

mculp · 2022-09-27T04:36:33Z

lib/eth/abi.rb

        # encodes dynamic-sized arrays
        head, tail = "", ""
-        head += encode_type Type.size_type, arg.size
+        head += encode_type(Type.size_type, arg.size)


+1 on the style change

mculp · 2022-09-27T04:41:03Z

lib/eth/abi.rb

      end
    end

+    def encode_struct_offsets(type, arg)


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

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? :)

lib/eth/abi/type.rb

lib/eth/client.rb

lib/eth/contract/function.rb

lib/eth/contract/function_input.rb

q9f · 2022-10-27T12:50:49Z

I'll be accepting this as is and resolve your comments on a new branch @mculp (I don't have write access here.)

peter-chung-xfers · 2022-10-27T18:36:40Z

@q9f @mculp

I don't really have time for this right now please take over thanks

Co-authored-by: Matt Culpepper <mculp@users.noreply.github.com>

unbanksy · 2022-10-29T15:45:46Z

lib/eth/abi/type.rb

        @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?


components.any? might fail if components = nil

unbanksy · 2022-10-29T21:33:11Z

lib/eth/abi.rb

+      raise EncodingError, "Expecting #{type.components.size} elements: #{arg}" unless arg.size == type.components.size
+
+      static_size = 0
+      type.components.each do |component|


Should be type.components.each_with_index do |component, i|

unbanksy · 2022-10-29T21:34:39Z

lib/eth/abi.rb

+        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])


Should be dynamic_values

q9f · 2022-12-21T17:19:43Z

Thank you @unbanksy - your comments were spot on.

I cannot edit this branch, so I took over in >> #185

* 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>

peter-chung-xfers added 4 commits July 8, 2022 02:41

add test case

7d84e6d

eth/abi: support dynamic struct encoding

18dfacf

remove unused function

6e2cfc4

typo

abf3448

peter-chung-xfers added 4 commits July 11, 2022 02:47

uncomment spec

ffc56a2

cleanup spec

905f3e2

test tuple2

5462190

add tuple2.sol

7bddbe8

q9f approved these changes Sep 26, 2022

View reviewed changes

q9f added the enhancement New feature or request label Sep 26, 2022

mculp reviewed Sep 27, 2022

View reviewed changes

peter-chung-xfers and others added 13 commits October 28, 2022 02:37

Update lib/eth/contract/function_input.rb

c78700c

Co-authored-by: Matt Culpepper <mculp@users.noreply.github.com>

Update lib/eth/client.rb

8b0831f

Co-authored-by: Matt Culpepper <mculp@users.noreply.github.com>

Update lib/eth/contract/function.rb

0ca7b6b

Co-authored-by: Matt Culpepper <mculp@users.noreply.github.com>

Update lib/eth/abi/type.rb

6b4ef53

Co-authored-by: Matt Culpepper <mculp@users.noreply.github.com>

Update lib/eth/abi.rb

e0bd279

Co-authored-by: Matt Culpepper <mculp@users.noreply.github.com>

Update lib/eth/abi.rb

75dcd1b

Co-authored-by: Matt Culpepper <mculp@users.noreply.github.com>

Update lib/eth/abi/type.rb

6e9c93b

Co-authored-by: Matt Culpepper <mculp@users.noreply.github.com>

Update lib/eth/abi/type.rb

fc26a60

Co-authored-by: Matt Culpepper <mculp@users.noreply.github.com>

Update lib/eth/abi/type.rb

9b29aa3

Co-authored-by: Matt Culpepper <mculp@users.noreply.github.com>

Update lib/eth/abi.rb

f5adabb

Co-authored-by: Matt Culpepper <mculp@users.noreply.github.com>

Update lib/eth/abi.rb

333aca1

Co-authored-by: Matt Culpepper <mculp@users.noreply.github.com>

Update lib/eth/abi.rb

1699cac

Co-authored-by: Matt Culpepper <mculp@users.noreply.github.com>

Update lib/eth/abi/type.rb

c567dc1

Co-authored-by: Matt Culpepper <mculp@users.noreply.github.com>

unbanksy reviewed Oct 29, 2022

View reviewed changes

q9f mentioned this pull request Dec 21, 2022

Dynamic struct encoding (#135) #185

Merged

q9f closed this Dec 21, 2022

Conversation

peter-chung-xfers commented Jul 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Jul 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

peter-chung-xfers commented Jul 18, 2022

Uh oh!

q9f left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

q9f commented Oct 27, 2022

Uh oh!

peter-chung-xfers commented Oct 27, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

q9f commented Dec 21, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

peter-chung-xfers commented Jul 10, 2022 •

edited

Loading

codecov-commenter commented Jul 10, 2022 •

edited

Loading