Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rewrite of tuple consensus-buff deserialization #434

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Acaccia
Copy link
Collaborator

@Acaccia Acaccia commented Jul 4, 2024

This is the first part of completing #307 .

This PR rewrites completely the tuple deserialization function to allow for the missing functionalities.
For now, the missing functionalities implemented are

  • fields can arrive in any order (not just lexicographic order)
  • fields cannot be duplicated

To satisfy those conditions, the new algorithm in pseudo-code became

let bitset = Bitset::new();
for key in serialized_bytes {
    let n = find_index(key)
    switch n {
        case 1:
            if bitset.contains(key) { return None; }
            handle_parsing_of_value_1;
            bitset.insert(key);
            break;
        case 2:
            if bitset.contains(key) { return None; }
            handle_parsing_of_value_2;
            bitset.insert(key);
            break;
        ...
        default:
            check_valid_skippable_key();
            check_valid_skippable_value();
            break;
    }
}
if bitset.full() { return Some(result) } else { return None };

These contains new functionalities that had to be created:

  1. The bitset is a list of i32 locals, the logic to handle it is directly inside the deserialization algo.
  2. The find_index function is a binary search in a list of string. It is implemented in the standard, with another function to compare clarity names (there are also unit tests for this function).
  3. The switch-case construct is a bit awkward in Wat, since it doesn't have _label_s and _goto_s instructions. So it's a succession of labelled blocks, with the inner one being the switch and using a br_table instruction to jump after the block labelled like the currently parsed field (I suggest compiling a few examples to see what it looks like).
  4. The default case where we switch extra fields is a TODO for now but is nearly complete already. I decided to cut it from this PR and add it in a future one because it will require quite a lot of code to handle the testing.

@Acaccia Acaccia requested review from csgui, krl and obycode July 4, 2024 19:33
@Acaccia Acaccia self-assigned this Jul 4, 2024
Copy link

codecov bot commented Jul 4, 2024

Codecov Report

Attention: Patch coverage is 95.87302% with 13 lines in your changes missing coverage. Please review.

Project coverage is 86.24%. Comparing base (93eace4) to head (438aec8).

Files Patch % Lines
clar2wasm/src/deserialize.rs 96.59% 7 Missing and 3 partials ⚠️
clar2wasm/src/wasm_generator.rs 85.71% 1 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #434      +/-   ##
==========================================
+ Coverage   86.20%   86.24%   +0.04%     
==========================================
  Files          44       44              
  Lines       19406    19551     +145     
  Branches    19406    19551     +145     
==========================================
+ Hits        16729    16862     +133     
- Misses       1212     1220       +8     
- Partials     1465     1469       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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.

None yet

1 participant