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

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

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,

eth/rlp/options.nim

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

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
+

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
+    doAssert self.pendingLists[lastListIdx].remainingItems > 0
-    if decRet(self.pendingLists[lastListIdx].remainingItems, 1) == 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
+  self.startList list.len
-  # nim bug #7416 is fixed.
+  for i in 0 ..< list.len:
-  when T is (byte or char):
+    self.append list[i]
-    self.appendBlob(listOrBlob)
-  else:
-    self.startList listOrBlob.len
-    for i in 0 ..< listOrBlob.len:
-      self.append listOrBlob[i]
 
-proc hasOptionalFields(T: type): bool =
+proc countOptionalFields(T: type): int {.compileTime.} =
   mixin enumerateRlpFields
 
-  proc helper: bool =
+  var dummy: T
-    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]
 
+  # closure signature matches the one in object_serialization.nim
   template op(RT, fN, f) =
-    res[i] = f is Option or f is Opt
+    when f is Option or f is Opt:
-    inc i
+      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
+  mixin enumerateRlpFields
-  type ObjType = type obj
 
-  const
+  var numOptionals: int = 0
-    fieldsCount = ObjType.rlpFieldsCount
-    # include first optional fields
-    cof = checkedOptionalFields(ObjType, fieldsCount) + 1
 
-  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)
+    cof = countOptionalFields(ObjType)
-    fieldsCount = ObjType.rlpFieldsCount
 
-  when hasOptional:
+  when cof > 0:
-    const
+    # ignoring first optional fields
-      cof = checkedOptionalFields(ObjType, fieldsCount)
+    genOptionalFieldsValidation(obj, ObjType, cof - 1)
-    when cof > 0:
-      genOptionalFieldsValidation(obj, ObjType, cof)
 
   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):
+  appendImpl(w, data)
-    # TODO detect negative enum values at compile time?
-    appendImpl(w, uint64(data))
-  else:
-    appendImpl(w, data)
 
 template append*(w: var RlpWriter; data: SomeSignedInt) =
   {.error: "Signed integer encoding is not defined for rlp".}

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

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