eth/types: add Num superclass and Hex, Dec, Bin descendants

[mculp]

Porting the Num type from https://github.com/q9f/secp256k1.cr/blob/master/src/secp256k1/num.cr

[q9f]

Thank you. I already had some opinions and left some comments.

[q9f]

Thank you, Matt.

I'm not very happy with how Ruby handles types. Therefore, I think using refinements should be avoided.

If we want to use the Num type throughout the library, we should handle all conversions internally in the Num class privately and only expose some getters for the different formats.

What do you think?

[codecov-commenter]

Codecov Report

Merging #76 (ee140a5) into main (7926fc4) will decrease coverage by 0.01%.
The diff coverage is 99.45%.

@@            Coverage Diff             @@
##             main      #76      +/-   ##
==========================================
- Coverage   99.74%   99.73%   -0.02%     
==========================================
  Files          67       76       +9     
  Lines        3923     4108     +185     
==========================================
+ Hits         3913     4097     +184     
- Misses         10       11       +1     

Impacted Files	Coverage Δ
spec/eth/types/num_spec.rb	93.75% <93.75%> (ø)
lib/eth.rb	100.00% <100.00%> (ø)
lib/eth/constant.rb	100.00% <100.00%> (ø)
lib/eth/types/bin.rb	100.00% <100.00%> (ø)
lib/eth/types/dec.rb	100.00% <100.00%> (ø)
lib/eth/types/hex.rb	100.00% <100.00%> (ø)
lib/eth/types/num.rb	100.00% <100.00%> (ø)
spec/eth/constant_spec.rb	100.00% <100.00%> (ø)
spec/eth/types/bin_spec.rb	100.00% <100.00%> (ø)
spec/eth/types/dec_spec.rb	100.00% <100.00%> (ø)
... and 2 more

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

[mculp]

Thank you, Matt.

I'm not very happy with how Ruby handles types. Therefore, I think using refinements should be avoided.

If we want to use the Num type throughout the library, we should handle all conversions internally in the Num class privately and only expose some getters for the different formats.

What do you think?

Just curious, what do you see as the issue with using refinements? A refinement is scoped to the file in which it is used. It's opt-in, file-by-file. So, as this PR stands, the only class that can see the changes to the String/Integer classes is Num. I was thinking we could add further conversions in the refinement (to_keccak, for example) and other classes could opt-in to using the refinement as they want.

If refinements are out, I think the next-best solution would be to subclass Num w/ Hex, Dec, Bin:

class Num
  attr_reader :hex, :dec, :bin
  
  def initialize(input = nil)
    generate_number
  end
  
  def to_bytes
    bin
  end
  
  def to_i
    dec
  end
  
  def to_hex
    hex
  end
end

class Hex < Num
  def initialize(input)
    @hex = input.delete_prefix('0x')
    @dec = hex.to_i(16)
    @bin = hex.scan(/../).map(&:hex).pack('C*').b
  end
end

Hex['0xasdfa'].to_bytes

I was going back and forth with this and the refinement solution; we can't do a straight port from Crystal because you can't define multiple constructors in Ruby. I did see you added a gem to do something like that, but the "multiple constructors" are actually just builder/factory methods if I understand it correctly. i.e. we'd have to do something like this: Num.from_hex('0xafsdfasdf') (we want something short and sweet) and there'd be a bunch of nested branching logic, which is typically avoided if possible in Ruby.

Now that I've looked at the code I typed above, using inheritance would probably be the most idiomatic solution out of the 3.

What do you think?

(btw, I'm not trying to be difficult. I'm just trying to solve this the simplest and most idiomatic way)

[q9f]

Now that I've looked at the code I typed above, using inheritance would probably be the most idiomatic solution out of the 3.

Yes, I love it. I'm not too concerned about the implications of the refinements, it's more about the style.

Sorry for being so pedantic. I think, your last suggestion most simple and clean approach.

[mculp]

Now that I've looked at the code I typed above, using inheritance would probably be the most idiomatic solution out of the 3.

Yes, I love it. I'm not too concerned about the implications of the refinements, it's more about the style.

Sorry for being so pedantic. I think, your last suggestion most simple and clean approach.

Awesome! What do you think if the files live in eth/types/... but then have their constants hoisted to Eth like Eth::Num = Eth::Types::Num, which would be shortened to Num in practice because everything is within the Eth namespace.

My original concern with the subclass approach was that I didn't want to pollute the top-level directory with a bunch of files, so I just rolled with the refinement since it's all technically self contained in Num.

[q9f]

This is looking good so far, do you need a hand to finish this off? :)

[mculp]

hey @q9f so sorry for the delay, I took a month vacation. Will finish this asap.

[mculp]

hey @q9f I made some updates, let me know what you think.

[mculp]

Is the CI broken? It says it's missing a token

[q9f]

Can you run rufo on the code? I will review asap.

Also, can you rebase on main branch? You pulled in a lot of commits unrelated to this work.

You can ignore the missing token issue on CI. It should be temporary.

self

[mculp]

@q9f I updated this with Rufo.

[q9f]

one test is failing, please check the CI

[mculp]

Going down a rabbit hole with this one test.

@q9f I should've asked this question a long time ago, but I noticed in the Crystal Num, it was essentially 3 types: Hex, Decimal, and an array of ASCII chars.

Do we want to do that here instead of this bytestring stuff I'm trying to do? The reason I chose a bytestring instead of an array of ascii ordinals is because the Keccak lib dumps/reads a bytestring.

Now:

    # @example Instantiate a Hex object
    #   Hex('0xdeadbeef')
    #     => #<Eth::Types::Hex:0x00007f8dcb102330
    #       @input="0xdeadbeef",
    #       @hex="deadbeef",
    #       @bin="\xDE\xAD\xBE\xEF",
    #       @dec=3735928559>

Would be much easier to deal with, but may not provide the same utility/helpfulness:

    # @example Instantiate a Hex object
    #   Hex('0xdeadbeef')
    #     => #<Eth::Types::Hex:0x00007f8dcb102330
    #       @input="0xdeadbeef",
    #       @hex="deadbeef",
    #       @bin=[51, 55, 51, 53, 57, 50, 56, 53, 53, 57],
    #       @dec=3735928559>

What do you think? This PR is starting to feel like a monkey on my back.

[q9f]

Haha, sorry for that. Please be creative, it's really not a high priority.

[q9f]

Let's bench this for now