Batch Merkle Proof Serialization & Deserialization #90
Conversation
| } | ||
| } | ||
|
|
||
| property("indices serialization + deserialization") { |
There was a problem hiding this comment.
I guess this test (along with proofs test below) is redundant. Should I remove?
Although perhaps I can force negative test scenarios here (whereas they would fail the bounds check as a BatchMerkleProof object).
There was a problem hiding this comment.
I left these tests in which directly use the private[serialization] methods. The BatchMerkleProoftest now uses the new serialize and deserialize methods.
| import scorex.crypto.authds.{EmptyByteArray, Side} | ||
| import scorex.crypto.hash.Digest | ||
|
|
||
| class BatchMerkleProofSerialization[D <: Digest] { |
There was a problem hiding this comment.
BatchMerkleProofSerialization => BatchMerkleProofSerializer
There was a problem hiding this comment.
D <: Digest => D <: Digest32 , as serialization assumes hashes of 256 bits.
Better to use DigestSize = 32 constant instead of magic value in the class ( DigestSize + 4 instead of 36), then it will be simpler to retarget for hashed of 512 bits if needed.
There was a problem hiding this comment.
Fixed, and went a bit further with the constants use to make the code more readable.
| }) | ||
| .toSeq | ||
| } | ||
| } |
There was a problem hiding this comment.
I think it is worth to make all the tests private[serialization] (to be used in tests only), and introduce serialize(b: ): Array[Byte] / deserialize(arr:Array[Byte): Try[BatchMerkleProof] methods. Please node that deserialization may faile if e.g. invalid bytes are provided. thus Try[BatchMerkleProof]. would be good to have a test for improper bytes provided (e.g. just 2 any bytes)
| } | ||
| } | ||
|
|
||
| property("indices serialization + deserialization") { |
Hi @kushti please review at your convenience. Let me know if any additional functionality is required.
Close #86