client decodes blocks before returning them

omgnetwork / plasma-mvp

OmiseGO's research implementation of Minimal Viable Plasma

MIT License

561 stars 158 forks source link

client decodes blocks before returning them #116

Closed eashend closed 6 years ago

eashend commented 6 years ago

This is meant to fix #100. The CLI no longer decodes blocks, rather the client decodes before returning. I was unsure if the child_chain should decode applied transactions and submitted blocks, but left this as is, as tests would fail otherwise.

Should decodes be moved out of child_chain along with the refactoring of tests to accommodate?

smartcontracts commented 6 years ago

This is awesome, thank you! It would definitely be useful to convert the rest of Client so that it only takes/returns full Block/Transaction objects as inputs/outputs. I think we're currently using encoded inputs or outputs in the following methods: apply_transaction, submit_block, withdraw, get_transaction.

We would have to refactor the tests too, but I think it'll end up being much cleaner than having to encode/decode everything outside of Client each time.

eashend commented 6 years ago

Okay, awesome! Thanks for the feedback, will update.

eashend commented 6 years ago

My previous thinking about test cases the child_chain may have been misguided.

The last commits pulled all encoding/decoding of blocks/tx for the child chain coming into and out of the client into client, this was my understanding of your previous comment .

Let me know if my understanding is correct on this or if I'm totally off base:

for client_chain_service to interact with the child chain server, I/O must be rlp encoded
- there must be an encode and a subsequent decode at some point between client and the child chain
the 'shortest' amount of time blocks/tx are encoded could be between the client_chain_service and the server
encoding/decoding can be taken out of the child_chain and into the server, and only at this point would the child chain tests need refactoring --- is this desirable?

eashend commented 6 years ago

Thanks for bearing with me, I believe this is what was originally called for, with the exception of withdraw which is still returning encoded to the root chain, and then using the RLP.sol library to decode.

smartcontracts commented 6 years ago

Yep, looks perfect! Thanks for the work on this.

DavidKnott commented 6 years ago

@eashend Thanks a lot! LGTM, merging.