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

Adapt to new CryptoStore API #148

Closed

Conversation

hirschenberger
Copy link

Copy link
Contributor

@adoerr adoerr left a comment

Choose a reason for hiding this comment

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

I think, we have a bit of a cyclic dependency issue here. My suggestion to resolve this is:

  • Merge the Substrate PR #8619 that updates the SyncCryptoStore API
  • We will bump the BEEFY Substrate dependency to a commit that does include this PR
  • The Polkadot companion PR should be done next

@tomusdrw
Copy link
Contributor

@adoerr instead of using branch for deps maybe let's use rev instead? That way we can update BEEFY repo before it lands in substrate master. I'm not willing to break regular substrate release process (merging despite companion failing) long term.

@adoerr
Copy link
Contributor

adoerr commented Apr 15, 2021

@tomusdrw using rev is an option. In the concrete case, however, I'm not sure this would buy us much. Substrate PR #8618 is still in review.

IMO, the issue here is that a Substrate API (SyncCryptoStore) has been changed, which is used by both, Polkadot and BEEFY. Polkadot depends on BEEFY as well.

Now, PR's to use the new version of the Substrate API have been opened, before the API change itself was even accepted to Substrate. IMO those two PR's should have done after the API change has been accepted into Substrate.

Or am I missing something here? Maybe, I just don't understand, how this Polkadot companion thing is supposed to work.

This is more efficient when using a remote signer (see paritytech/substrate#7285)
@ordian
Copy link
Member

ordian commented Apr 15, 2021

I don't see a cyclic dependency (BEEFY doesn't depend on polkadot). It seems for now, we'll have to merge such PRs manually (substrate -> beefy -> polkadot). Long-term, merging BEEFY into substrate/polkadot repo would solve this problem (or making bot aware of BEEFY).

@tomusdrw
Copy link
Contributor

The main problem is that the companion set up was introduced to make sure that random Substrate PRs don't introduce extra work for Polkadot developers - i.e. every change in Substrate that is breaking Polkadot must have a PR (companion PR) that fixes any incompatibilities. This is ENFORCED by Substrate CI - i.e. the build has to be green before you can merge the PR.

Now with the way BEEFY is integrated right now, if there is a Substrate change that breaks BEEFY code there is no way to create a Polkadot companion PR that would turn green (there is one, but it requires patching all beefy deps in Polkadot, which is quite cumbersome). So the most straightforward solution is to either:

  1. Merge Substrate PR with failing Polkadot companion and coordinate on swift update.
  2. Do something backward compatible (as suggested here)
  3. Change the process to involve BEEFY repo as well.
  4. Create a workaround with patching in Polkadot.
  5. Consider Remove branch = "master" from toml files  #149 to be able to freely switch branches (i.e. depend only on revision).

Long term it's going to be solved by the merge indeed or proper versioning/crates.io deployment, but we should discuss how to deal with such breaking changes short/mid-term.

@tomusdrw
Copy link
Contributor

@hirschenberger good news, we are planning to expand the companion PR system to this repo now, so it should be possible to move forward soonish.

@adoerr
Copy link
Contributor

adoerr commented Sep 27, 2021

Since BEEFY is now part of Substrate, this PR has become obsolete.

@adoerr adoerr closed this Sep 27, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants