Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Adding Bridges code as git subtree. #2515

Merged
merged 8 commits into from
Mar 1, 2021
Merged

Adding Bridges code as git subtree. #2515

merged 8 commits into from
Mar 1, 2021

Conversation

tomusdrw
Copy link
Contributor

@tomusdrw tomusdrw commented Feb 24, 2021

This PR adds entire https://github.com/paritytech/parity-bridges-common repository as git subtree.

The goal of having Bridges code in Polkadot repo is to make it possible to integrate with polkadot-based runtimes (Rococo & Westend first, later Polkadot & Kusama), while achieving following properties:

  1. Keeping Bridges & Polkadot development process independent and focused (especially reducing notifications fatigue).
  2. Reducing maintenance costs for Polkadot developers (coordinating substrate version bumps or substrate version changes).

I expect this integration will have a bunch of bumps on it's way, but it's still the best what we get to satisfy above conditions. I know this is something we have never really tried before and the PR is quite significant, despite that I feel it's worth to try.

How to work with git subtree?

Some FAQ

Why git subtree?

We did a short overview of possible solutions internally, and a summary can be found below (ordered by how closely the code is integrated):
  1. Depending on crates.io versioning - not feasibly for now, given our fast development pace.
  2. Using git-based dependencies - high maintenance cost for substrate devs (any breaking change requires PRs to bridges repo first, before the companion PR can be merged).
  3. Using git submodule - this may lead to including unreviewed (neither in Polkadot nor in Bridges) code (just referencing commit hash of bridges repo) and may be hard to track versioning-wise (i.e. "Does this hash contain that important change or is it done on some other fork of the code").
  4. Using git subtree - the code is duplicated in both repos, there might be some unused/unreferenced code lingering in Polkadot repo.
  5. Merging the repos - merging development practices, mixing notification/issues/prs which might make it hard to navigate and move fast.

Why not remove unused bridges files?

I expect this will make it much harder to contribute changes upsteam, cause you may run into many conflicts due to files being removed. I only removed `Cargo.toml`, because nested workspaces confuse `cargo`, but since it's just one file I don't think it will cause much trouble.

What about BEEFY / other projects

Bridges are already quite big and complex and break quite often when Substrate changes. I expect BEEFY to be referenced using git-based cargo dependencies short term and integrated with Substrate repo long term.

Why not integrate BRIDGES with Substrate repo?

Similar arguments apply if we were to integrate bridges to Substrate (see "Why git subtree?"). Moreover bridges have quite a lot of chain-specific components, which I feel makes them even less of a fit to generic framework repository.

@github-actions github-actions bot added the A0-please_review Pull request needs code review. label Feb 24, 2021
@tomusdrw tomusdrw added B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Feb 24, 2021
BRIDGES.md Outdated Show resolved Hide resolved
BRIDGES.md Outdated Show resolved Hide resolved
BRIDGES.md Outdated Show resolved Hide resolved
BRIDGES.md Outdated Show resolved Hide resolved
Comment on lines +29 to +31
"bridges/primitives/kusama",
"bridges/primitives/polkadot",
"bridges/primitives/runtime",
Copy link
Contributor

Choose a reason for hiding this comment

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

This reminds me - we have a branch called bridges-integration which already adds some Polkadot and Kusama primitives. While it's a bit outdated (last commit was Dec 22nd), we should probably use it as a reference for integration moving forward.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I'll add integration in a separate PR cause it should be properly reviewed. This one is 65k lines of code that was already reviewed in bridges repo, hence the separation.

BRIDGES.md Show resolved Hide resolved
Copy link
Contributor

@andresilva andresilva left a comment

Choose a reason for hiding this comment

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

I don't really have any strong opinions on this, I haven't done any research on it anyway so I just trust your judgment. Either way, I think this is the way to go in the sense that we should just test this approach and if it doesn't work we can revert it later, otherwise there will be opinions floating around with nothing getting decided in practice.

Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

What @andresilva says

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants