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

chore: reduce redundancy with msgpack #1108

Merged
merged 8 commits into from
Jul 19, 2023

Conversation

ludamad
Copy link
Collaborator

@ludamad ludamad commented Jul 18, 2023

Description

Going through lists-of-fields now that we have poor-man's reflection.

This currently DOES NOT switch barretenberg to msgpack. This instead uses msgpack common reflection utils to derive such read and write functions.

For example:
MSGPACK_FIELDS(arg1, arg2);

is now used to define

void read(buf, obj) {
   read(buf, obj.arg1);
   read(buf, obj.arg2);
}

for any object that has a MSGPACK_FIELDS macro. This is also used for write

Also removes a debug utility I introduced during the hackathon that has not really been used, fine to remove

Also see AztecProtocol/barretenberg#612
There's still read and write methods that can be done similarly.

Checklist:

  • [*] I have reviewed my diff in github, line by line.
  • [*] Every change is related to the PR description.
  • [-] I have linked this pull request to the issue(s) that it resolves.

    I should have created an issue first in hindsight, sorry

  • [*] There are no unexpected formatting changes, superfluous debug logs, or commented-out code.
  • [*] The branch has been merged or rebased against the head of its merge target.
  • [*] I'm happy for the PR to be merged at the reviewer's next convenience.

Copy link
Collaborator

@spalladino spalladino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Over 550 lines deleted. Gotta love this kind of PRs!

using serialize::read;

read(it, new_contract_data.contract_address);
read(it, new_contract_data.portal_contract_address.address_);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like here we're reading/writing directly to a field within this struct, which is not the same as msgpack is doing I guess. Is this change ok?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was never needed I believe, see read(portal_contract_address), confusing though!

template <typename B> void read(B& it, address& addr)
{
    using serialize::read;
    fr address_field;
    read(it, address_field);
    addr = address(address_field);
}

=> 
    address(fr const& address)
        : address_(address){};

Copy link
Contributor

@jeanmon jeanmon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Great to remove superfluous code.

@ludamad ludamad enabled auto-merge (squash) July 19, 2023 15:39
Copy link
Contributor

@dbanks12 dbanks12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome. Lgtm

@ludamad ludamad disabled auto-merge July 19, 2023 16:57
@ludamad ludamad enabled auto-merge (squash) July 19, 2023 16:57
@ludamad ludamad merged commit e287d03 into master Jul 19, 2023
@ludamad ludamad deleted the ad/chore/reduce-redundancy-msgpack branch July 19, 2023 18:27
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.

4 participants