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: move protobuf module from utils to common #1390

Merged
merged 1 commit into from
Nov 16, 2022
Merged

Conversation

LNSD
Copy link
Contributor

@LNSD LNSD commented Nov 16, 2022

Following previous work with sqlite.nim and envvar_serialziation.nim modules, this PR moves the protobuf module to the common module. This module should, in the future, act as a "facade" for the protobuf codec APIs.

  • Move protobuf.nim under common module
  • Re-export libp2p's protobuf codec APIs
  • Update imports accordingly

@LNSD LNSD self-assigned this Nov 16, 2022
@status-im-auto
Copy link
Collaborator

status-im-auto commented Nov 16, 2022

Jenkins Builds

Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ f01590d #1 2022-11-16 13:38:48 ~15 min macos 📦bin
✔️ f01590d #1 2022-11-16 13:42:46 ~19 min linux 📦bin

Copy link
Contributor

@jm-clius jm-clius left a comment

Choose a reason for hiding this comment

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

Great! Worth adding a line to the Coding Guide re "use common/protobuf and don't import libp2p minprotobuf directly"?

Copy link
Contributor

@alrevuelta alrevuelta left a comment

Choose a reason for hiding this comment

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

Using @jm-clius guideline "use common/protobuf and don't import libp2p minprotobuf directly"?, perhaps we are missing updating waku_peer_storage.nim as well?

We can leave that for another PR, but do we also want to remove libp2p/protobuf/minprotobuf from the tests?

@LNSD
Copy link
Contributor Author

LNSD commented Nov 16, 2022

Great! Worth adding a line to the Coding Guide re "use common/protobuf and don't import libp2p minprotobuf directly"?

Note that I have only touched the protocol modules. So... I am not 100% sure if we should do it. I have not analyzed the impact of moving everything to import this common module.

I had in mind to do that when we decided the protobuf library (see: #1361)

Using @jm-clius guideline "use common/protobuf and don't import libp2p minprotobuf directly"?, perhaps we are missing updating waku_peer_storage.nim as well?

I am unfamiliar with waku_peer_store.nim; that's why I left it out. To minimize the risk. The waku_peer_store needs a cleanup. Changing that, which will change in the near future, IMO, is out of the scope of this PR.

We can leave that for another PR, but do we also want to remove libp2p/protobuf/minprotobuf from the tests?

Same thing here. I don't know the implication of removing the types. And I am pretty sure that, in some cases, the analysis will lead us to a deeper refactoring of the test suite. I want to avoid this extra work to be part of this PR.

Copy link
Contributor

@rymnc rymnc left a comment

Choose a reason for hiding this comment

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

LGTM!

@LNSD
Copy link
Contributor Author

LNSD commented Nov 16, 2022

The check failure can be ignored. It is not related to the changes.

The offending test case is already tracked in #1357.

@LNSD LNSD merged commit e2a2ea6 into master Nov 16, 2022
@LNSD LNSD deleted the chore-common-protobuf branch November 16, 2022 19:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants