Implemented missing data structure for serialization and deserialization of external transition #202

sudo-shashank · 2022-04-05T20:24:24Z

Summary of changes
Changes introduced in this pull request:

Reference issue to close (if applicable)

Closes #194
#143 except: test: Implement test_snapp_command().

Other information and links

codecov-commenter · 2022-04-07T09:35:10Z

Codecov Report

Merging #202 (bb7e2ea) into main (29b121d) will increase coverage by 0.94%.
The diff coverage is 95.00%.

@@            Coverage Diff             @@
##             main     #202      +/-   ##
==========================================
+ Coverage   86.58%   87.52%   +0.94%     
==========================================
  Files         101      103       +2     
  Lines        4471     4682     +211     
==========================================
+ Hits         3871     4098     +227     
+ Misses        600      584      -16

Impacted Files	Coverage Δ
base/src/lib.rs	`100.00% <ø> (ø)`
base/src/snark_work.rs	`0.00% <0.00%> (ø)`
base/src/staged_ledger_diff.rs	`93.10% <0.00%> (-3.33%)`	⬇️
...col/serialization-types/src/external_transition.rs	`100.00% <ø> (ø)`
protocol/serialization-types/src/lib.rs	`100.00% <ø> (ø)`
base/src/serialization_type_conversions/mod.rs	`99.00% <100.00%> (+3.22%)`	⬆️
protocol/bin-prot/src/ser.rs	`87.50% <100.00%> (+6.37%)`	⬆️
protocol/serialization-types/src/snark_work.rs	`100.00% <100.00%> (ø)`
...ocol/serialization-types/src/staged_ledger_diff.rs	`86.36% <100.00%> (+5.41%)`	⬆️
protocol/bin-prot/src/read_ext.rs	`95.89% <0.00%> (-1.37%)`	⬇️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 29b121d...bb7e2ea. Read the comment docs.

hanabi1224 · 2022-04-08T09:51:46Z

Amazing work, just one thing, is it possible to get block(s) that can exercise uncovered serde code? It looks like the overall codecov drop by ~3% with this change

willemolding

Amazing work dude! Just needs a tidy up and we are good

base/src/numbers.rs

base/src/snark_work.rs

willemolding · 2022-04-22T03:24:27Z

protocol/bin-prot/src/ser.rs

-            self.writer.bin_write_variant_index(b)?;
+    // Use variant name to identify polyvar type, else sum type
+    fn write_variant_index_or_tag(&mut self, name: &str, index: u32, variant: &str) -> Result<()> {
+        if name == "Polyvar" {


Did we not already merge this stuff?

willemolding · 2022-04-22T03:25:18Z

protocol/serialization-types/src/external_transition.rs

@@ -37,5 +37,5 @@ pub struct ExternalTransition {
 pub struct ExternalTransitionV1(pub Versioned<ExternalTransition, 1>);

 impl bin_prot::encodable::BinProtEncodable for ExternalTransitionV1 {
-    const PREALLOCATE_BUFFER_BYTES: usize = 13 * 1024;
+    const PREALLOCATE_BUFFER_BYTES: usize = 13 * 1024; // FIXME: This size is not suitable for blocks with snark work data


I suggest removing this trait entirely since it is only a helper for writing tests. Feel free to increase the buffer to make things work (but it might break again if we get a larger block)

increased the buffer size for now, to support all the sample test blocks we have

willemolding · 2022-04-22T03:27:05Z

protocol/serialization-types/tests/test-serialization.rs

@@ -439,6 +449,7 @@ fn test_staged_ledger_diff_diff_commands_status() {
 #[wasm_bindgen_test]
 fn test_staged_ledger_diff_diff_coinbase() {
    block_path_test_batch! {
+        // FIXME: The given path is not valid for all test blocks


This test makes invalid assumptions (that the variant is always the second one in

pub enum CoinBase { Zero, // FIXME: other variants are not covered by current test block One(Option<CoinBaseFeeTransferV1>), Two, }

This is invalid so we should just remove the test entirely. It is covered by the next line anyway

Yeah, I think it's safe to disable the test for now since we have e2e coverage, ideally, the logic here should be, deserializing the binary into all variants and asserting there is exactly one being successful

Check out #218

willemolding · 2022-04-22T03:27:33Z

protocol/serialization-types/tests/test-serialization.rs

        CoinBaseBalanceDataV1 => "t/staged_ledger_diff/t/diff/t/0/t/t/internal_command_balances/0/t/[sum]"
+        // FIXME: The given path is not valid for all test blocks


Same as above

protocol/serialization-types/tests/test_deserialize_snark_work.rs

protocol/serialization-types/tests/test_deserialize_staged_ledger_pre_diff_one.rs

protocol/test-fixtures/src/lib.rs

protocol/test-serialization/src/test_blocks_de.rs

hanabi1224 · 2022-04-25T08:07:54Z

base/src/external_transition.rs

@@ -28,3 +28,7 @@ pub struct ExternalTransition {
 impl BinProtSerializationType for ExternalTransition {
    type T = ExternalTransitionV1;
 }
+
+impl bin_prot::encodable::BinProtEncodable for ExternalTransition {


This should have been removed

hanabi1224 · 2022-04-26T05:47:00Z

It's strange that when I run the tests from #221 in this PR, CI jobs time out, there might be some perf issue to investigate

creativcoder · 2022-04-27T10:29:41Z

protocol/serialization-types/tests/test-serialization.rs

        }
    };
 }

+#[derive(Debug, Clone, Serialize, Deserialize)]


can we add a comment describing why we need this dummy struct for later reference?

yeah i'll add it

sudo-shashank requested review from willemolding, creativcoder, noot and hanabi1224 as code owners April 5, 2022 20:24

sudo-shashank changed the title ~~Implemented Transaction Snark Data Structure~~ WIP: Implemented Transaction Snark Data Structure Apr 6, 2022

sudo-shashank force-pushed the shashank/#194 branch 5 times, most recently from 75b6e47 to 07ae35f Compare April 7, 2022 09:10

sudo-shashank changed the title ~~WIP: Implemented Transaction Snark Data Structure~~ Implemented Transaction Snark Data Structure Apr 7, 2022

sudo-shashank added the ready for review label Apr 7, 2022

sudo-shashank added the Do not merge Status: Do not merge Added to PRs that are not allowed to be merged label Apr 11, 2022

sudo-shashank changed the title ~~Implemented Transaction Snark Data Structure~~ WIP: Implemented Transaction Snark Data Structure Apr 11, 2022

sudo-shashank removed the ready for review label Apr 11, 2022

sudo-shashank force-pushed the shashank/#194 branch 3 times, most recently from b5be92f to 4a0b53a Compare April 12, 2022 12:19

sudo-shashank changed the title ~~WIP: Implemented Transaction Snark Data Structure~~ Implemented missing data structure for serialization and deserialization of external transition Apr 12, 2022

sudo-shashank force-pushed the shashank/#194 branch 3 times, most recently from dabd9ba to a6cdb12 Compare April 20, 2022 03:37

sudo-shashank linked an issue Apr 21, 2022 that may be closed by this pull request

[M4] Test coverages staged ledger diff #143

Closed

5 tasks

willemolding reviewed Apr 22, 2022

View reviewed changes

sudo-shashank force-pushed the shashank/#194 branch 2 times, most recently from eab18d1 to 12d1534 Compare April 22, 2022 09:57

sudo-shashank removed Do not merge Status: Do not merge Added to PRs that are not allowed to be merged labels Apr 22, 2022

sudo-shashank added ready for review and removed Do not merge Status: Do not merge Added to PRs that are not allowed to be merged labels Apr 22, 2022

sudo-shashank force-pushed the shashank/#194 branch from 12d1534 to 5ecac40 Compare April 25, 2022 07:13

hanabi1224 reviewed Apr 25, 2022

View reviewed changes

sudo-shashank force-pushed the shashank/#194 branch from 5ecac40 to 11a1154 Compare April 25, 2022 09:11

hanabi1224 approved these changes Apr 26, 2022

View reviewed changes

hanabi1224 mentioned this pull request Apr 26, 2022

M1 - Block serde e2e roundtrip tests #221

Closed

creativcoder reviewed Apr 27, 2022

View reviewed changes

sudo-shashank requested a review from creativcoder April 27, 2022 11:27

sudo-shashank force-pushed the shashank/#194 branch from d2d860c to c5bb937 Compare April 27, 2022 11:30

creativcoder approved these changes Apr 28, 2022

View reviewed changes

sudo-shashank force-pushed the shashank/#194 branch 2 times, most recently from 05f258f to b9d8e53 Compare May 3, 2022 04:17

sudo-shashank and others added 10 commits May 3, 2022 17:58

Implemented Transaction Snark and StagedLedgerDiffOne Data Structure

eae66d6

Fixed polyvar serialization and added some tests

c1b38ae

Enabled test for all sample blocks

09a1082

diagnose msgs for testing

aa14fb6

block name in err msg

4e238d8

Handle empty variant case in block_sum_path_test

55b181d

e2e tests to increase codecov

9b07e91

test_block_serde_roundtrip

fdf4877

Fix bad perf in tests

83006ab

added comment for block sum path test

e1dae7b

sudo-shashank force-pushed the shashank/#194 branch from b9d8e53 to e1dae7b Compare May 3, 2022 12:54

sudo-shashank merged commit 855c8de into main May 3, 2022

sudo-shashank deleted the shashank/#194 branch May 3, 2022 14:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented missing data structure for serialization and deserialization of external transition #202

Implemented missing data structure for serialization and deserialization of external transition #202

sudo-shashank commented Apr 5, 2022 •

edited

Loading

codecov-commenter commented Apr 7, 2022 •

edited

Loading

hanabi1224 commented Apr 8, 2022

willemolding left a comment

willemolding Apr 22, 2022

willemolding Apr 22, 2022

sudo-shashank Apr 22, 2022

willemolding Apr 22, 2022

hanabi1224 Apr 22, 2022

hanabi1224 Apr 22, 2022

willemolding Apr 22, 2022

hanabi1224 Apr 25, 2022

hanabi1224 commented Apr 26, 2022 •

edited

Loading

creativcoder Apr 27, 2022

sudo-shashank Apr 27, 2022

		CoinBaseBalanceDataV1 => "t/staged_ledger_diff/t/diff/t/0/t/t/internal_command_balances/0/t/[sum]"
		// FIXME: The given path is not valid for all test blocks

Implemented missing data structure for serialization and deserialization of external transition #202

Implemented missing data structure for serialization and deserialization of external transition #202

Conversation

sudo-shashank commented Apr 5, 2022 • edited Loading

codecov-commenter commented Apr 7, 2022 • edited Loading

Codecov Report

hanabi1224 commented Apr 8, 2022

willemolding left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hanabi1224 commented Apr 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sudo-shashank commented Apr 5, 2022 •

edited

Loading

codecov-commenter commented Apr 7, 2022 •

edited

Loading

hanabi1224 commented Apr 26, 2022 •

edited

Loading