Skip to content

Fix header RLP encoding in mock code#376

Merged
majecty merged 5 commits intoCodeChain-io:masterfrom
majecty:f/fix-mock-header-2
May 12, 2020
Merged

Fix header RLP encoding in mock code#376
majecty merged 5 commits intoCodeChain-io:masterfrom
majecty:f/fix-mock-header-2

Conversation

@majecty
Copy link

@majecty majecty commented May 7, 2020

This PR depends on #369

@majecty majecty requested a review from sgkim126 May 7, 2020 09:56
@majecty majecty force-pushed the f/fix-mock-header-2 branch from 8e298fb to 274d1e1 Compare May 7, 2020 10:26
sgkim126
sgkim126 previously approved these changes May 9, 2020
@sgkim126 sgkim126 added the do-not-merge Do not merge this PR label May 9, 2020
@sgkim126
Copy link
Contributor

sgkim126 commented May 9, 2020

LGTM. Let's remove the do-not-merge label after #369 merged.

@majecty majecty force-pushed the f/fix-mock-header-2 branch 3 times, most recently from aa8cc43 to 80a9e3e Compare May 11, 2020 04:51
Park Juhyung added 5 commits May 11, 2020 14:03
The seal is an array of any RLP data. Wrapping them with Buffer makes
invalid RLP encoding.
Since Header has a cache of its hash using RefCell, the existence of
the cache changes the compare result.
@majecty majecty force-pushed the f/fix-mock-header-2 branch from 80a9e3e to 1c03372 Compare May 11, 2020 05:04
@majecty
Copy link
Author

majecty commented May 11, 2020

@sgkim126 I fixed some test cases.
Since I removed PartialEq from Header, I couldn't use rlp_encode_and_decode_test macro. I used the Debug trait instead to compare two structs. Do you think is it okay? It's a weird solution, but I couldn't find a better solution.
ffb2ce0#diff-2253d387d6c10db9b770115468aa0d92R177


    /// For a type that does not have PartialEq, uses Debug instead.
    fn assert_eq_by_debug<T: std::fmt::Debug>(a: &T, b: &T) {
        assert_eq!(format!("{:?}", a), format!("{:?}", b));
    }

@majecty majecty removed the do-not-merge Do not merge this PR label May 11, 2020
@majecty majecty requested a review from sgkim126 May 11, 2020 06:10
majecty pushed a commit to majecty/foundry that referenced this pull request May 11, 2020
Change mock's Header::default function to static function

Change seal serialize code in mock

The seal is an array of any RLP data. Wrapping them with Buffer makes
invalid RLP encoding.

Add Header RLP encoding test in mock

Remove PartialEq derive from Header and Block

Since Header has a cache of its hash using RefCell, the existence of
the cache changes the compare result.

Add a hint comment that helps programmer to manage tests
@majecty
Copy link
Author

majecty commented May 12, 2020

The failed test is "connect 100 nodes". I'll merge this PR.

@majecty majecty merged commit 739da0c into CodeChain-io:master May 12, 2020
@majecty majecty deleted the f/fix-mock-header-2 branch May 12, 2020 01:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants