status-im / nim-eth

Common utilities for Ethereum
https://nimbus.status.im
Apache License 2.0
83 stars 31 forks source link

clean up redundant code in eth/rlp/writer.nim #755

Closed chirag-parmar closed 3 weeks ago

chirag-parmar commented 1 month ago

Changelog

Rationale

arnetheduck commented 1 month ago

since you're here, another "discrepancy" is that https://github.com/status-im/nim-eth/blob/master/eth/rlp/options.nim#L12 exists as a helper for std/options but isn't used - a similar helper exists somewhere in nimbus for Opt and is actually used, but then in rlp itself there's some half-way Opt/Option support also, which is pretty confusing.

We're moving away from std/options broadly (in fact, I don't think we use them any more in rlp), so to give options a proper cleanup, this should also be attended to so that Opt always gets the same treatment (likely, all support for Opt should live in the rlp library proper but @jangko might know better).

jangko commented 1 month ago

Opt/Options often get special treatment in our encoding library. e.g. json-serialization, json-rpc, and rlp. What I mean special is not the encoding process of the field content itself, but prior to that. When d/encoding an object, the library need to decide what to do with the optional fields. So there are two parts regarding optional fields, (1) field content encoding, overrideable, and (2) handling optional fields of an object is coded in library. But I agree rlp/options.nim can be removed.

example in json-serialization: https://github.com/status-im/nim-json-serialization/blob/6eadb6e939ffa7882ff5437033c11a9464d3385c/json_serialization/writer.nim#L91-L99

example in json-rpc: https://github.com/status-im/nim-json-rpc/blob/98a5efba4de26ac852d0715656f6b0c52a203a75/json_rpc/private/server_handler_wrapper.nim#L138-L154

chirag-parmar commented 1 month ago

Someone thought about this before https://github.com/status-im/nim-eth/blob/master/eth/common/base_rlp.nim#L14

chirag-parmar commented 3 weeks ago

https://github.com/status-im/nimbus-eth1/actions/runs/11679712586 https://github.com/status-im/nimbus-eth1/actions/runs/11679712567