Are the protocol messages supposed to extend RLPValue? How about going more decoupled way, maybe something like json encoders/decoders (typeclasses) in Play! framework (and "clean" message classes)?

Ditto on that.
We would then create new typeclass instances in RLPImplicits (or in the encoded type's companion object) for whatever we need to encode.

And while on implicits, why do we need the implicit conversion in the other directions, i.e. RLPEncodable -> primitive types?

@LukasGasior1 about:

Are the protocol messages supposed to extend RLPValue? How about going more decoupled way, maybe something like json encoders/decoders (typeclasses) in Play! framework (and "clean" message classes)?

Ok, we will take a second look at it. We first tried type classes but the code got dirty very quickly. Feel free to point us some example / code regarding this if you want to.

@rtkaczyk not sure if I understand:

And while on implicits, why do we need the implicit conversion in the other directions, i.e. RLPEncodable -> primitive types?

You need that in order to decode messages to known types (for example this PingMessage example)

@AlanVerbner
I mean something in this way:

trait RLPEncoder[T] {
	def encode(obj: T): RLPValue
}

trait RLPDecoder[T] {
	def decode(rlp: RLPValue): T
}


case class Ping(...)

object Ping {
	implicit val encDec = new RLPEncoder[Ping] with RLPDecoder[Ping] {
		override def encode = ...
		override def decode = ...	
	}
}

then you can just have encode (and decode) method:

def encode[T: RLPEncoder](obj: T): RLPValue

capable of encoding any T as long as there's implicit encoder in scope

// edit: encode should probably use RLP.encode and return the final byte array instead, but that's just an example

@rtkaczyk ^ does that make sense?

@LukasGasior1 yes, that's also what I was thinking. RLPEncoder/RLPDecoder companions should provide some primitives for encoding simple values, that the instances would rely on.

@LukasGasior1 @rtkaczyk Just pushed the improvements you suggested. Please let us know what do you think 😄

@AlanVerbner I think that's a step in good direction :)

It seems RLPImplicits are not really implicits ATM:

 override def encode(obj: PingMessage): RLPEncodeable = new RLPList {
        override def items: Seq[RLPEncodeable] = Seq(
          intEncDec.encode(obj.version),
          Endpoint.encDec.encode(obj.from),
          Endpoint.encDec.encode(obj.to),
          longEncDec.encode(obj.expiration)
        )
      }

it would be way better if we could just write:

override def encode(obj: PingMessage) = {
  import obj._  
  RLPList(version, from, to, expiration)
}

this should be easily doable

Ok, will take another look @LukasGasior1 😄

@LukasGasior1 I guess it's done now 😄

For the protocol handshake (frame codec actually) I'm also going to need following RLP-related methods (from ethereumj):

public static RLPElement decode2OneItem(byte[] msgData, int startPos) {
        RLPList rlpList = new RLPList();
        fullTraverse(msgData, 0, startPos, startPos + 1, 1, rlpList);
        return rlpList.get(0);
    }

and something like:

RLP.getNextElementIndex(buffer, pos)

see:
https://github.com/ethereum/ethereumj/blob/develop/ethereumj-core/src/main/java/org/ethereum/net/rlpx/FrameCodec.java#L195 and
https://github.com/ethereum/ethereumj/blob/develop/ethereumj-core/src/main/java/org/ethereum/net/rlpx/FrameCodec.java#L166

@AlanVerbner
would it be difficult to add these in your implementation?

@LukasGasior1 checking it atm

@LukasGasior1 about decode2OneItem our RLP implementation only decodes one single item at a time so it's already supported if you send the array starting at the correct position. For example, the following test taken from EthereumJ

 @Test
public void partialDataParseTest() {
    String hex = "000080c180000000000000000000000042699b1104e93abf0008be55f912c2ff";
    RLPList el = (RLPList) decode2OneItem(Hex.decode(hex), 3);
    assertEquals(1, el.size());
    assertEquals(0, Util.rlpDecodeInt(el.get(0)));
}

will be translated as:

test("Partial Data Parse Test") {
    val hex: String = "000080c180000000000000000000000042699b1104e93abf0008be55f912c2ff"
    val data = Hex.decode(hex)
    val decoded: Try[Seq[Int]] = RLP.decode[Seq[Int]](data.splitAt(3)._2)
    assert(decoded.isSuccess)
    assert(1 == decoded.get.length)
    assert(0 == decoded.get.head)
  }

We can change the method signature in order to avoid doing the split before calling the rlp decoder you can call something like

val decoded: Try[Seq[Int]] = RLP.decode[Seq[Int]](data, 3)

That being said, it's important to note that our implementation does not support decoding arrays made like encode(msg1) ++ encode(anotherMessage) because those are not items of a single RLPList. Does it make sense?

@AlanVerbner Sure it does. Didn't know it already works that way :) No need to change the signature, data.drop(index) is sufficient. And how about RLP.getNextElementIndex(buffer, pos)?
https://github.com/ethereum/ethereumj/blob/develop/ethereumj-core/src/main/java/org/ethereum/net/rlpx/FrameCodec.java#L195

@LukasGasior1 RLP.getNextElementIndex(buffer, pos) it's not supported atm but easily doable. We are trying to find a way to avoid repeating that code (in EthereumJ is at least done 3 times if i'm not mistaken)...will get back to you with that coded 😄

Thanks, hopefully that will be all we need on rlp side :)

@LukasGasior1 Sorry for the delay but tests were messing around with me. I've just pushed RLP.getNextElementIndex(buffer, pos). There are several tests starting from here

Please check if it fits networking needs. Also let me know if you want me to create tests for encoding / decoding handshake massages.

Thanks, LGTM 👍

@AlanVerbner Can you, if you didn't already add tests for the 2 fixes you mention? Ta!

@mcsherrylabs I've already did but will try to add a couple more if those are needed