From 034b7886de225705c80ff1e822040cacb553aeee Mon Sep 17 00:00:00 2001 From: Chirag Parmar Date: Wed, 6 Nov 2024 10:16:22 +0530 Subject: [PATCH] clean up redundant code in eth/rlp/writer.nim (#755) * cleanup macros * add test cases and fix counting function * add check statements * remove deprecated support for Option * replace some logic * remove debug print --- eth/common/base_rlp.nim | 21 +--- eth/rlp.nim | 13 --- eth/rlp/options.nim | 17 --- eth/rlp/results.nim | 22 ++++ eth/rlp/writer.nim | 168 +++++++++++------------------ tests/rlp/all_tests.nim | 3 +- tests/rlp/test_optional_fields.nim | 68 ++++++++++++ 7 files changed, 156 insertions(+), 156 deletions(-) delete mode 100644 eth/rlp/options.nim create mode 100644 eth/rlp/results.nim create mode 100644 tests/rlp/test_optional_fields.nim diff --git a/eth/common/base_rlp.nim b/eth/common/base_rlp.nim index d4c9f46..4765cfa 100644 --- a/eth/common/base_rlp.nim +++ b/eth/common/base_rlp.nim @@ -7,30 +7,17 @@ {.push raises: [].} -import std/typetraits, ./base, ../rlp +import + std/typetraits, ./base, ../rlp, + ../rlp/results as rlp_results -export base, rlp +export base, rlp, rlp_results -# TODO why is rlp serialization of `Opt` here and not in rlp? -proc append*[T](w: var RlpWriter, val: Opt[T]) = - mixin append - - if val.isSome: - w.append(val.get()) - else: - w.append("") template read*[T](rlp: var Rlp, val: var T) = mixin read val = rlp.read(type val) -proc read*[T](rlp: var Rlp, val: var Opt[T]) {.raises: [RlpError].} = - mixin read - if rlp.blobLen != 0: - val = Opt.some(rlp.read(T)) - else: - rlp.skipElem - proc read*(rlp: var Rlp, T: type StUint): T {.raises: [RlpError].} = if rlp.isBlob: let bytes = rlp.toBytes diff --git a/eth/rlp.nim b/eth/rlp.nim index f01214f..610e767 100644 --- a/eth/rlp.nim +++ b/eth/rlp.nim @@ -448,9 +448,6 @@ func readImpl( else: rlp.bytes.len() - template getUnderlyingType[T](_: Option[T]): untyped = - T - template getUnderlyingType[T](_: Opt[T]): untyped = T @@ -458,16 +455,6 @@ func readImpl( type FieldType {.used.} = type field when hasCustomPragmaFixed(RecordType, fieldName, rlpCustomSerialization): field = rlp.read(result, FieldType) - elif field is Option: - # this works for optional fields at the end of an object/tuple - # if the optional field is followed by a mandatory field, - # custom serialization for a field or for the parent object - # will be better - type UT = getUnderlyingType(field) - if rlp.position < payloadEnd: - field = some(rlp.read(UT)) - else: - field = none(UT) elif field is Opt: # this works for optional fields at the end of an object/tuple # if the optional field is followed by a mandatory field, diff --git a/eth/rlp/options.nim b/eth/rlp/options.nim deleted file mode 100644 index f67b454..0000000 --- a/eth/rlp/options.nim +++ /dev/null @@ -1,17 +0,0 @@ -import - std/options, - ../rlp - -proc read*[T](rlp: var Rlp, O: type Option[T]): O {.inline.} = - mixin read - if not rlp.isEmpty: - result = some read(rlp, T) - -proc append*(writer: var RlpWriter, value: Option) = - if value.isSome: - writer.append value.get - else: - writer.append "" - -export - options, rlp diff --git a/eth/rlp/results.nim b/eth/rlp/results.nim new file mode 100644 index 0000000..f2d3a81 --- /dev/null +++ b/eth/rlp/results.nim @@ -0,0 +1,22 @@ +import ../rlp +import writer +import pkg/results + +export + rlp, results + +proc append*[T](w: var RlpWriter, val: Opt[T]) = + mixin append + + if val.isSome: + w.append(val.get()) + else: + w.append("") + +proc read*[T](rlp: var Rlp, val: var Opt[T]) {.raises: [RlpError].} = + mixin read + if rlp.blobLen != 0: + val = Opt.some(rlp.read(T)) + else: + rlp.skipElem + diff --git a/eth/rlp/writer.nim b/eth/rlp/writer.nim index e726ddc..56362c2 100644 --- a/eth/rlp/writer.nim +++ b/eth/rlp/writer.nim @@ -1,6 +1,6 @@ import std/options, - results, + pkg/results, stew/[arraybuf, assign2, bitops2, shims/macros], ./priv/defs @@ -8,7 +8,7 @@ export arraybuf type RlpWriter* = object - pendingLists: seq[tuple[remainingItems, outBytes: int]] + pendingLists: seq[tuple[remainingItems, startPos: int]] output: seq[byte] RlpIntBuf* = ArrayBuf[9, byte] @@ -41,7 +41,7 @@ func writeCount(bytes: var auto, count: int, baseMarker: byte) = origLen = bytes.len lenPrefixBytes = uint64(count).bytesNeeded - bytes.setLen(origLen + int(lenPrefixBytes) + 1) + bytes.setLen(origLen + lenPrefixBytes + 1) bytes[origLen] = baseMarker + (THRESHOLD_LIST_LEN - 1) + byte(lenPrefixBytes) bytes.writeBigEndian(uint64(count), bytes.len - 1, lenPrefixBytes) @@ -60,17 +60,16 @@ proc initRlpWriter*: RlpWriter = # expected to be short-lived, it doesn't hurt to allocate this buffer result.output = newSeqOfCap[byte](2000) -proc decRet(n: var int, delta: int): int = - n -= delta - n - proc maybeClosePendingLists(self: var RlpWriter) = while self.pendingLists.len > 0: let lastListIdx = self.pendingLists.len - 1 - doAssert self.pendingLists[lastListIdx].remainingItems >= 1 - if decRet(self.pendingLists[lastListIdx].remainingItems, 1) == 0: + doAssert self.pendingLists[lastListIdx].remainingItems > 0 + + self.pendingLists[lastListIdx].remainingItems -= 1 + # if one last item is remaining in the list + if self.pendingLists[lastListIdx].remainingItems == 0: # A list have been just finished. It was started in `startList`. - let listStartPos = self.pendingLists[lastListIdx].outBytes + let listStartPos = self.pendingLists[lastListIdx].startPos self.pendingLists.setLen lastListIdx # How many bytes were written since the start? @@ -104,33 +103,21 @@ proc appendRawBytes*(self: var RlpWriter, bytes: openArray[byte]) = self.output.len - bytes.len, self.output.len - 1), bytes) self.maybeClosePendingLists() -proc appendRawList(self: var RlpWriter, bytes: openArray[byte]) = - self.output.writeCount(bytes.len, LIST_START_MARKER) - self.appendRawBytes(bytes) - proc startList*(self: var RlpWriter, listSize: int) = if listSize == 0: - self.appendRawList([]) + self.output.writeCount(0, LIST_START_MARKER) + self.appendRawBytes([]) else: self.pendingLists.add((listSize, self.output.len)) -proc appendBlob(self: var RlpWriter, data: openArray[byte], startMarker: byte) = +proc appendBlob(self: var RlpWriter, data: openArray[byte]) = if data.len == 1 and byte(data[0]) < BLOB_START_MARKER: self.output.add byte(data[0]) self.maybeClosePendingLists() else: - self.output.writeCount(data.len, startMarker) + self.output.writeCount(data.len, BLOB_START_MARKER) self.appendRawBytes(data) -proc appendImpl(self: var RlpWriter, data: string) = - appendBlob(self, data.toOpenArrayByte(0, data.high), BLOB_START_MARKER) - -proc appendBlob(self: var RlpWriter, data: openArray[byte]) = - appendBlob(self, data, BLOB_START_MARKER) - -proc appendBlob(self: var RlpWriter, data: openArray[char]) = - appendBlob(self, data.toOpenArrayByte(0, data.high), BLOB_START_MARKER) - proc appendInt(self: var RlpWriter, i: SomeUnsignedInt) = # this is created as a separate proc as an extra precaution against # any overloading resolution problems when matching the IntLike concept. @@ -138,64 +125,47 @@ proc appendInt(self: var RlpWriter, i: SomeUnsignedInt) = self.maybeClosePendingLists() + +template appendImpl(self: var RlpWriter, data: openArray[byte]) = + self.appendBlob(data) + +template appendImpl(self: var RlpWriter, data: openArray[char]) = + self.appendBlob(data.toOpenArrayByte(0, data.high)) + +template appendImpl(self: var RlpWriter, data: string) = + self.appendBlob(data.toOpenArrayByte(0, data.high)) + template appendImpl(self: var RlpWriter, i: SomeUnsignedInt) = - appendInt(self, i) + self.appendInt(i) template appendImpl(self: var RlpWriter, e: enum) = - appendImpl(self, int(e)) + # TODO: check for negative enums + self.appendInt(uint64(e)) template appendImpl(self: var RlpWriter, b: bool) = - appendImpl(self, int(b)) + self.appendInt(uint64(b)) -proc appendImpl[T](self: var RlpWriter, listOrBlob: openArray[T]) = +proc appendImpl[T](self: var RlpWriter, list: openArray[T]) = mixin append - # TODO: This append proc should be overloaded by `openArray[byte]` after - # nim bug #7416 is fixed. - when T is (byte or char): - self.appendBlob(listOrBlob) - else: - self.startList listOrBlob.len - for i in 0 ..< listOrBlob.len: - self.append listOrBlob[i] + self.startList list.len + for i in 0 ..< list.len: + self.append list[i] -proc hasOptionalFields(T: type): bool = +proc countOptionalFields(T: type): int {.compileTime.} = mixin enumerateRlpFields - proc helper: bool = - var dummy: T - result = false - template detectOptionalField(RT, n, x) {.used.} = - when x is Option or x is Opt: - return true - enumerateRlpFields(dummy, detectOptionalField) - - const res = helper() - return res - -proc optionalFieldsNum(x: openArray[bool]): int = - # count optional fields backward - for i in countdown(x.len-1, 0): - if x[i]: inc result - else: break - -proc checkedOptionalFields(T: type, FC: static[int]): int = - mixin enumerateRlpFields - - var - i = 0 - dummy: T - res: array[FC, bool] + var dummy: T + # closure signature matches the one in object_serialization.nim template op(RT, fN, f) = - res[i] = f is Option or f is Opt - inc i + when f is Option or f is Opt: + inc result + else: # this will count only optional fields at the end + result = 0 enumerateRlpFields(dummy, op) - # ignoring first optional fields - optionalFieldsNum(res) - 1 - proc genPrevFields(obj: NimNode, fd: openArray[FieldDescription], hi, lo: int): NimNode = result = newStmtList() for i in countdown(hi, lo): @@ -230,32 +200,21 @@ macro genOptionalFieldsValidation(obj: untyped, T: type, num: static[int]): unty doAssert obj.blobGasUsed.isSome == obj.excessBlobGas.isSome, "blobGasUsed and excessBlobGas must both be present or absent" -macro countFieldsRuntimeImpl(obj: untyped, T: type, num: static[int]): untyped = - let - Tresolved = getType(T)[1] - fd = recordFields(Tresolved.getImpl) - res = ident("result") - mlen = fd.len - num - - result = newStmtList() - result.add quote do: - `res` = `mlen` - - for i in countdown(fd.high, fd.len-num): - let fieldName = fd[i].name - result.add quote do: - `res` += `obj`.`fieldName`.isSome.ord - proc countFieldsRuntime(obj: object|tuple): int = - # count mandatory fields and non empty optional fields - type ObjType = type obj + mixin enumerateRlpFields - const - fieldsCount = ObjType.rlpFieldsCount - # include first optional fields - cof = checkedOptionalFields(ObjType, fieldsCount) + 1 + var numOptionals: int = 0 - countFieldsRuntimeImpl(obj, ObjType, cof) + template op(RT, fN, f) {.used.} = + when f is Option or f is Opt: + if f.isSome: # if optional and non empty + inc numOptionals + else: # if mandatory field + inc result + numOptionals = 0 # count only optionals at the end (after mandatory) + + enumerateRlpFields(obj, op) + result += numOptionals proc appendRecordType*(self: var RlpWriter, obj: object|tuple, wrapInList = wrapObjsInList) = mixin enumerateRlpFields, append @@ -263,25 +222,22 @@ proc appendRecordType*(self: var RlpWriter, obj: object|tuple, wrapInList = wrap type ObjType = type obj const - hasOptional = hasOptionalFields(ObjType) - fieldsCount = ObjType.rlpFieldsCount + cof = countOptionalFields(ObjType) - when hasOptional: - const - cof = checkedOptionalFields(ObjType, fieldsCount) - when cof > 0: - genOptionalFieldsValidation(obj, ObjType, cof) + when cof > 0: + # ignoring first optional fields + genOptionalFieldsValidation(obj, ObjType, cof - 1) if wrapInList: - when hasOptional: + when cof > 0: self.startList(obj.countFieldsRuntime) else: - self.startList(fieldsCount) + self.startList(ObjType.rlpFieldsCount) template op(RecordType, fieldName, field) {.used.} = when hasCustomPragmaFixed(RecordType, fieldName, rlpCustomSerialization): append(self, obj, field) - elif (field is Option or field is Opt) and hasOptional: + elif (field is Option or field is Opt) and cof > 0: # this works for optional fields at the end of an object/tuple # if the optional field is followed by a mandatory field, # custom serialization for a field or for the parent object @@ -293,20 +249,16 @@ proc appendRecordType*(self: var RlpWriter, obj: object|tuple, wrapInList = wrap enumerateRlpFields(obj, op) -proc appendImpl(self: var RlpWriter, data: object) {.inline.} = +template appendImpl(self: var RlpWriter, data: object) = self.appendRecordType(data) -proc appendImpl(self: var RlpWriter, data: tuple) {.inline.} = +template appendImpl(self: var RlpWriter, data: tuple) = self.appendRecordType(data) # We define a single `append` template with a pretty low specificity # score in order to facilitate easier overloading with user types: template append*[T](w: var RlpWriter; data: T) = - when data is (enum|bool): - # TODO detect negative enum values at compile time? - appendImpl(w, uint64(data)) - else: - appendImpl(w, data) + appendImpl(w, data) template append*(w: var RlpWriter; data: SomeSignedInt) = {.error: "Signed integer encoding is not defined for rlp".} diff --git a/tests/rlp/all_tests.nim b/tests/rlp/all_tests.nim index 783b27d..625768c 100644 --- a/tests/rlp/all_tests.nim +++ b/tests/rlp/all_tests.nim @@ -2,4 +2,5 @@ import ./test_api_usage, ./test_json_suite, ./test_empty_string, - ./test_object_serialization + ./test_object_serialization, + ./test_optional_fields diff --git a/tests/rlp/test_optional_fields.nim b/tests/rlp/test_optional_fields.nim new file mode 100644 index 0000000..c234c0c --- /dev/null +++ b/tests/rlp/test_optional_fields.nim @@ -0,0 +1,68 @@ +{.used.} + +import + ../../eth/[rlp, common], + unittest2 + +# Optionals in between mandatory fields for the convenience of +# implementation. According to the spec all optionals appear +# after mandatory fields. Moreover, an empty optional field +# cannot and will not appear before a non-empty optional field + +type ObjectWithOptionals = object + a* : uint64 + b* : uint64 + c* : Opt[uint64] # should not count this as optional + d* : Opt[uint64] # should not count this as optional + e* : uint64 + f* : uint64 + g* : uint64 + h* : Opt[uint64] # should not count this as optional + i* : Opt[uint64] # should not count this as optional + j* : Opt[uint64] # should not count this as optional + k* : uint64 + l* : Opt[uint64] # should count this as an optional + m* : Opt[uint64] # should count this as an optional + n* : Opt[uint64] # should count this as an optional + +var + objWithEmptyOptional: ObjectWithOptionals + objWithNonEmptyOptional: ObjectWithOptionals + objWithNonEmptyTrailingOptionals: ObjectWithOptionals + objWithEmptyTrailingOptionals: ObjectWithOptionals + +objWithNonEmptyOptional.c = Opt.some(0'u64) +objWithNonEmptyOptional.d = Opt.some(0'u64) +objWithNonEmptyOptional.h = Opt.some(0'u64) +objWithNonEmptyOptional.i = Opt.some(0'u64) +objWithNonEmptyOptional.j = Opt.some(0'u64) +objWithNonEmptyOptional.l = Opt.some(0'u64) +objWithNonEmptyOptional.m = Opt.some(0'u64) +objWithNonEmptyOptional.n = Opt.some(0'u64) + +objWithNonEmptyTrailingOptionals.l = Opt.some(0'u64) +objWithNonEmptyTrailingOptionals.m = Opt.some(0'u64) +objWithNonEmptyTrailingOptionals.n = Opt.some(0'u64) + +objWithEmptyTrailingOptionals.c = Opt.some(0'u64) +objWithEmptyTrailingOptionals.d = Opt.some(0'u64) +objWithEmptyTrailingOptionals.h = Opt.some(0'u64) +objWithEmptyTrailingOptionals.i = Opt.some(0'u64) +objWithEmptyTrailingOptionals.j = Opt.some(0'u64) + +suite "test optional fields": + test "all optionals are empty": + let bytes = rlp.encode(objWithEmptyOptional) + check: bytes.len == 7 # 6 mandatory fields + prefix byte + + test "all optionals are non empty": + let bytes = rlp.encode(objWithNonEmptyOptional) + check: bytes.len == 15 # 6 mandatory + 8 optional + prefix + + test "Only trailing optionals are non empty": + let bytes = rlp.encode(objWithNonEmptyTrailingOptionals) + check: bytes.len == 10 # 6 mandatory + 3 trailing optional + prefix + + test "Only trailing optionals are empty": + let bytes = rlp.encode(objWithEmptyTrailingOptionals) + check: bytes.len == 12 # 6 mandatory + 5 non trailing + prefix