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

[chirag-parmar]

Changelog

• renamed checkedOptionalFields to countOptionalFields and removed unnecessary abstractions(optionalFieldsNum) for counting optional fields
• removes redundant hasOptional in favor of countOptionalFields.
• removed macro countFieldsRuntimeImpl in favor of the nimbus style guide.

Rationale

• Logical comparison on booleans (x == True) is equivalent to that of integers (x > 0) therefore hasOptional was redundant
• All the counting of optional fields anyways happened in runtime(op of enumerateRlpFields) all that was required was an additional check for emptiness of optional values. Hence the macro countFieldsRuntimeImpl was not required.

[jangko]

unorthodox but clever.

[arnetheduck]

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]

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]

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

[arnetheduck]

exports go on top, below the import, not sure why they are here

[chirag-parmar]

Good point. I'll change it.

[arnetheduck]

options.nim is so named after the module it provides support for - this file should thus be renamed to results.nim

[chirag-parmar]

Ah makes sense. I'll change it

[chirag-parmar]

nim-lang/Nim#2819 (comment)

[chirag-parmar]

@arnetheduck I have wasted too much time for this simple issue. Having the same name as the package is causing an error "cannot import itself". I tried to look for the rules of qualification and landed on the above issue. Its significantly old so maybe you know a way to qualify the package nim-results in an import.

[arnetheduck]

pkg/results and ./results

[chirag-parmar]

🙈

[chirag-parmar]

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