search

status-im / nim-serialization

A modern and extensible serialization framework for Nim
63 stars 8 forks source link

Allow defining only a reader #3

Open mratsim opened 5 years ago

mratsim commented 5 years ago

For the state tests I'm defining only a Bitfield reader but it apparently triggered issues:

WRN 2019-04-30 14:36:39-06:00 Error while handling RLPx message          topics="rlpx" tid=6523 err="\nAsync traceback:\n  beacon_node.nim(794)         beacon_node\n  beacon_node.nim(717)         start\n  beacon_node.nim(690)         run\n  asyncloop.nim(822)           ru
    runForever\n  asyncloop.nim(266)           poll\n  asyncmacro2.nim(39)          dispatchMessages_continue\n  rlpx.nim(533)                dispatchMessagesIter\n  rlpx.nim(266)                invokeThunk\n  asyncmacro2.nim(306)         emit_thunk\n  asyncmacro2.nim(36)
im(36)          emit_thunk_continue\n  rlpx.nim(802)                emit_thunkIter\n  asyncmacro2.nim(306)         emit\n  asyncmacro2.nim(36)          emit_continue\n  gossipsub_protocol.nim(65)   emitIter\n  serialization.nim(65)        :anonymous\n  serialization.nim(4
.nim(46)        readValue\n  reader.nim(181)              readValue\n  object_serialization.nim(90) :anonymous\n  bitfield.nim(17)             readValue\n  serialization.nim(46)        readValue\n  reader.nim(111)              readValue\n  reader.nim(91)               req
   requireToken\n  reader.nim(64)               raiseUnexpectedToken\n  #[\n    beacon_node.nim(794)         beacon_node\n    beacon_node.nim(717)         start\n    beacon_node.nim(690)         run\n    asyncloop.nim(822)           runForever\n    asyncloop.nim(266)
)           poll\n    asyncmacro2.nim(39)          dispatchMessages_continue\n    rlpx.nim(533)                dispatchMessagesIter\n    rlpx.nim(266)                invokeThunk\n    asyncmacro2.nim(306)         emit_thunk\n    asyncmacro2.nim(39)          emit_thunk_cont
k_continue\n    rlpx.nim(802)                emit_thunkIter\n    asyncfutures2.nim(377)       read\n  ]#\nException message: \nException type:" msg=emit peer=Node[127.0.0.1:50007] node=0

cc @arnetheduck

Those were solved by commenting out the reader in https://github.com/status-im/nim-beacon-chain/pull/259

This would be helpful for YAML serialization so that we can forego the writer implementation.

arnetheduck commented 5 years ago

well, it stands to reason that when you introduce a custom reader, you also need a corresponding writer so that the serialization round-trips, no?

mratsim commented 5 years ago

Is it really necessary? For configuration file like .ini, .toml or in your case for .yaml tests we don't need to produce them.

From a time-to-market point-of-view, it saves half the work (differences between parsing and generation aside).

arnetheduck commented 5 years ago

well, it's necessary if you write :) that code failed because it was calling that serializer and trying to read back on the other end (broadcasts etc) :yin_yang:

mratsim commented 5 years ago

Mmmh right I think I conflated 2 issues:

  1. Reading and writing types - the Bitfield case.
  2. Reading and writing format (i.e. de/serialization of yaml, ini, toml, ...)

We had a writer missing on case 1 leading to a bug and I assumed that for case 2 (format) we would have the same issue.

So the question is "can we implement only a reading for a serialization format without the corresponding writer?", if yes we can close the issue.

In short the context is wrong but the question is valid :D