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

Merged network-primitives into network package #7722

Merged
merged 7 commits into from
Sep 29, 2022

Conversation

pompon0
Copy link
Contributor

@pompon0 pompon0 commented Sep 29, 2022

Part of the public API of network package was placed in near-network-primitives and part was in near-network. Hence most of the other packages imported both anyway. Merging them back together allows us to:

  • decrease the public API surface (since within a crate visibility settings are more flexible)
  • put API next to the only implementation of that API
  • decrease the confusion when importing network stuff in other packages

@pompon0 pompon0 requested a review from matklad September 29, 2022 10:46
@pompon0 pompon0 requested a review from a team as a code owner September 29, 2022 10:46
Copy link
Contributor

@matklad matklad left a comment

Choose a reason for hiding this comment

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

To give more context here: the x / x-primitives split historically goes along our public RPC boundary. But there's just a couple of types which atually are on the RPC bounday, and we just moved them over to client-primitives.

Comment on lines +630 to +631
type From = near_network::types::AccountOrPeerIdOrHash;
type To = near_client_primitives::types::AccountOrPeerIdOrHash;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice pattern, going to add it to my toolbox!

//! WARNING WARNING WARNING
//! We need to maintain backwards compatibility, all changes to this file needs to be reviews.
use crate::network_protocol::edge::{Edge, PartialEdgeInfo};
use crate::network_protocol::{PeerChainInfoV2, PeerInfo, RoutedMessage};
Copy link
Contributor

Choose a reason for hiding this comment

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

should we kill borsh already?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried recently, but our python test suite is very messy and it would take me a few weeks to remove borsh usage there. I don't have that capacity now.

@near-bulldozer near-bulldozer bot merged commit 1eefdb8 into master Sep 29, 2022
@near-bulldozer near-bulldozer bot deleted the gprusak-remove-primitives branch September 29, 2022 12:39
nikurt pushed a commit that referenced this pull request Nov 9, 2022
Part of the public API of network package was placed in near-network-primitives and part was in near-network. Hence most of the other packages imported both anyway. Merging them back together allows us to:
- decrease the public API surface (since within a crate visibility settings are more flexible)
- put API next to the only implementation of that API
- decrease the confusion when importing network stuff in other packages
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