Make rlp module more readable and organized with tests. #40
Conversation
I am yet to add testcases for this module and also integrate test fixtures for this module. @lightclient @SamWilsn @adietrichs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll take another look once the code is restructured :)
@SamWilsn @lightclient @adietrichs I have added some personal tests and also integrated ethereum tests fixtures, and all of them seem to be working. Can you please review the code? Thanks. |
@SamWilsn @lightclient @adietrichs can you please review the code? Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is so much more readable! Great work. Just a bunch of me nitpicking really.
I did a few spot checks on the RLP test data, but we should probably make an issue to look into some differential fuzzing against other implementations (and/or steal their test suites.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, nice job Somu! I left a few comments, but I think the big takeaways are:
- Where possible, name the magic RLP lengths - and when defining them, use hex notation. Almost everyone interacts with RLP in hex representation, so it's just natural to think of these things using their hex values.
- Avoid too much parameterization of tests and focus on readability instead.
- Also fixes potential bugs in rlp module - Add tests and integrate with ethereum test fixtures
Codecov Report
@@ Coverage Diff @@
## pyspec #40 +/- ##
==========================================
+ Coverage 84.08% 92.40% +8.31%
==========================================
Files 13 13
Lines 754 750 -4
==========================================
+ Hits 634 693 +59
+ Misses 120 57 -63
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty happy with this, but I'd wait for @lightclient's review before merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry for being so nitpicky - I just think it's really important to use hex values as much we can when dealing with RLP since RLP is almost always viewed in that form. To most devs, numbers like 183, 192, etc. mean nothing to them. But things like 0xb7
and 0xc0
have meaning.
What was wrong?
The
rlp
module was closely structured to the yellowpaper making it hard to comprehend and maintain. Also there were a few potential bugs in the code which were cleaned up. One such code bug is as follows.Related to Issue #18
How was it fixed?
The code was rewritten using
Encoder
andDecoder
classes to get clarity. The bugs have also been fixed.Cute Animal Picture