From fe06a15ca0cea4790f964774bd4111754fc754f8 Mon Sep 17 00:00:00 2001 From: Matija Salopek Date: Thu, 27 Apr 2023 17:29:42 +0200 Subject: [PATCH 1/3] docs: draft adr-004 for separating consumer and provider versioning --- .../adrs/adr-004-separation-of-concerns.md | 106 ++++++++++++++++++ 1 file changed, 106 insertions(+) create mode 100644 docs/docs/adrs/adr-004-separation-of-concerns.md diff --git a/docs/docs/adrs/adr-004-separation-of-concerns.md b/docs/docs/adrs/adr-004-separation-of-concerns.md new file mode 100644 index 0000000000..15f189b36c --- /dev/null +++ b/docs/docs/adrs/adr-004-separation-of-concerns.md @@ -0,0 +1,106 @@ +--- +sidebar_position: 4 +title: Separating consumer and provider libraries +--- +# ADR 004: Separating consumer and provider libraries + +## Changelog +* 2023-04-27: Initial draft + +## Status + +> A decision may be "proposed" if it hasn't been agreed upon yet, or "accepted" once it is agreed upon. If a later ADR changes or reverses a decision, it may be marked as "deprecated" or "superseded" with a reference to its replacement. + +{Deprecated|Proposed|Accepted} + +## Context +The main deliverable for interchain-security are consumer and provider libraries that are currently located in `x/ccv/provider` and `x/ccv/consumer`. +Any blockchain applications available in the interchain-security repository are used for testing purposes only and should be viewed as examples for building chains implementing ICS. + +By cosmos-sdk terminology and conventions, all applications in the interchain-security repos are just `simapp`s: +* `interchain-security-pd` is a `simapp` for testing the provider ICS library +* `interchain-security-cd` is a `simapp` for testing the consumer ICS library + +Any apps existing in the interchain-security repository should not be considered as deliverables as they are not intended to be used by ICS customers, aside from usage in e2e testing frameworks. + +Splitting consumer and provider libraries allows different release cycles and makes development easier while maintaining provider and consumer compatibility. +Versioning library code shared between the consumer and provider arises as an intermediary step in the process of splitting the provider and consumer concerns. +By versioning the shared provider and consumer code we benefit from not having to lock the provider and consumer to the same dependencies at all times. + +## Handling shared functionality +At present provider and consumer libraries share some functionality, type, interfaces and constants. + +Most of the shared funcionality comes from the fact that the provider chain has to communicate the initial validator set to the consumer so that the consumer can include it in its genesis.json (consumer has to know the state of the provider's validator set when the first consumer block is produced). The remainder of shared functionality comprises ICS messaging types and their validation. + +It is important to notice that most of the shared types originate from .proto files. The .proto types are not versioned in `main` which means that any change to .proto types affects both consumer and provider. At present, the consumer and the provider cannot deviate from one another in any way. This is a hinderance and a bottleneck, since work on consumer and provider libraries can be paralelized by introducing a clean split between provider and consumer libraries. + + +### Build tooling incompatibility prevents rapid development +cosmos-sdk v0.45 and v0.47 use different build tools: +* https://docs.cosmos.network/v0.47/tooling/protobuf +* https://docs.cosmos.network/v0.47/migrations/upgrading#protobuf + +`interchain-security` is only half way migrated to the tooling required for `cosmos-sdk v0.47`. Even though ICS has been upgraded to use `buf` the dependency on `gogo/protobuf` remains because `cosmos-sdk v0.45` requires it. Meanwhile, `cosmos-sdk v0.47` has migrated to use [cosmos/gogoproto](https://github.com/cosmos/gogoproto). + +Besides that, ICS relies on older build tool images (ghcr.io/cosmos/proto-builder). To be compatible with `cosmos-sdk v0.47` ICS needs to upgrade the proto build image to `ghcr.io/cosmos/proto-builder >= 0.11.5`. + +### Versioning shared dependencies +To enable smooth upgrade from cosmos-sdk `v0.45` to `v0.47` the proto build tooling must be upgraded. The proto build tooling cannot be upgraded incrementally, it has to be upgraded all at once. +To mitigate issues and preserve the QA for both provider and consumer the shared dependencies should be versioned (tagged). + +Shared dependencies will be moved to a separate `core` module that will be built using the proto tooling intended for its corresponding `cosmos-sdk` version. + +Initially, we create two `core` module tags: +* `core-45/0.1.0` -> uses legacy build tooling and `cosmos-sdk v0.45` (tag name is subject to change) +* `core-47/0.1.0` -> uses new build tooling and `cosmos-sdk v0.47` (tag name is subject to change) + +With these changes we will make the shared dependencies available for cosmos-sdk `v0.45` and `v0.47`. This will allow us to add features to ICS versions that still use `cosmos-sdk v0.45` in case we need to perform emergency procedures for ICS versions `<= 1.2.x`. + +Any work using `cosmos-sdk v0.47` will be uninterrupted by emergency upgrades to older versions. + +### QA considerations +We rely on the QA process to assure that provider and consumer are compatible. Breaking the QA will essentially leave us in the dark for a prolonged period of time. We would be doing lots of changes without having a way to reliably ensure that we are not introducing regressions and breaking the core protocol. + +Besides that, we cannot allow for `main` to be in a broken state for prolonged periods of time because an emergency release would be very difficult to execute. Since emergency releases do happen (as has been shown with dragonberry and changes in ICS regarding consumer chain removals) we cannot simply ignore the systemic risk they introduce. Our workflows must be adapted in a way that accounts for emergency scenarios, to the best of our ability. + +### Future prospects +In the future, it is likely that P&C will be locked to the same dependency versions for 90% of the time. However, having the option to remain compatible while performing dependency upgrades will significantly speed up development on both sides. + +Changes in build tooling without versioning the `core` module would affect both C and P which would preclude the split. + +## Steps to perform +The proto build tooling has changed significantly between cosmos-sdk `v0.45` and cosmos-sdk `v0.47`. Due to the fact that `main` currently uses a legacy version of the proto build tool chain, simply upgrading the toolchain breaks both provider and consumer. + +The changes to proto build system cannot go unaddressed, and they should be addressed first in order to allow smoother development of both provider and consumer. + +1. separate out the `core` module that comprises types, functions and interfaces shared by provider and consumer +2. version (tag) the `core-v45` module using legacy proto build tooling (`cosmos-sdk v0.45`, `gogo/protobuf`, `ghcr.io/cosmos/proto-builder >= 0.9.0`) +3. version (tag) the `core-v47` module using latest protobuild tooling (`cosmos-sdk v0.47`, `cosmos/gogoproto`, `ghcr.io/cosmos/proto-builder >= 0.11.5`) +4. use `core-v45` in the current implementations of provider and consumer libraries +5. use `core-v47` in the future implementation provider and consumer libraries + + +## Decision + +> This section explains all of the details of the proposed solution, including implementation details. +It should also describe affects / corollary items that may need to be changed as a part of this. +If the proposed change will be large, please also indicate a way to do the change to maximize ease of review. +(e.g. the optimal split of things to do between separate PR's) + +## Consequences + +> This section describes the consequences, after applying the decision. All consequences should be summarized here, not just the "positive" ones. + +### Positive +TODO +### Negative +TODO + +### Neutral +TODO + +## References + +> Are there any relevant PR comments, issues that led up to this, or articles referrenced for why we made the given design choice? If so link them here! + +* {reference link} From 75151979544962ebbe9e14d8c3852dc08a335b27 Mon Sep 17 00:00:00 2001 From: Shawn <44221603+smarshall-spitzbart@users.noreply.github.com> Date: Fri, 28 Apr 2023 14:51:00 -0700 Subject: [PATCH 2/3] formatting --- .../docs/adrs/adr-004-separation-of-concerns.md | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/docs/docs/adrs/adr-004-separation-of-concerns.md b/docs/docs/adrs/adr-004-separation-of-concerns.md index 15f189b36c..e790168133 100644 --- a/docs/docs/adrs/adr-004-separation-of-concerns.md +++ b/docs/docs/adrs/adr-004-separation-of-concerns.md @@ -5,7 +5,9 @@ title: Separating consumer and provider libraries # ADR 004: Separating consumer and provider libraries ## Changelog + * 2023-04-27: Initial draft +* 2023-04-28: Changed content to reflect new approach ## Status @@ -14,10 +16,12 @@ title: Separating consumer and provider libraries {Deprecated|Proposed|Accepted} ## Context + The main deliverable for interchain-security are consumer and provider libraries that are currently located in `x/ccv/provider` and `x/ccv/consumer`. Any blockchain applications available in the interchain-security repository are used for testing purposes only and should be viewed as examples for building chains implementing ICS. By cosmos-sdk terminology and conventions, all applications in the interchain-security repos are just `simapp`s: + * `interchain-security-pd` is a `simapp` for testing the provider ICS library * `interchain-security-cd` is a `simapp` for testing the consumer ICS library @@ -28,6 +32,7 @@ Versioning library code shared between the consumer and provider arises as an in By versioning the shared provider and consumer code we benefit from not having to lock the provider and consumer to the same dependencies at all times. ## Handling shared functionality + At present provider and consumer libraries share some functionality, type, interfaces and constants. Most of the shared funcionality comes from the fact that the provider chain has to communicate the initial validator set to the consumer so that the consumer can include it in its genesis.json (consumer has to know the state of the provider's validator set when the first consumer block is produced). The remainder of shared functionality comprises ICS messaging types and their validation. @@ -36,7 +41,9 @@ It is important to notice that most of the shared types originate from .proto fi ### Build tooling incompatibility prevents rapid development + cosmos-sdk v0.45 and v0.47 use different build tools: + * https://docs.cosmos.network/v0.47/tooling/protobuf * https://docs.cosmos.network/v0.47/migrations/upgrading#protobuf @@ -45,12 +52,14 @@ cosmos-sdk v0.45 and v0.47 use different build tools: Besides that, ICS relies on older build tool images (ghcr.io/cosmos/proto-builder). To be compatible with `cosmos-sdk v0.47` ICS needs to upgrade the proto build image to `ghcr.io/cosmos/proto-builder >= 0.11.5`. ### Versioning shared dependencies + To enable smooth upgrade from cosmos-sdk `v0.45` to `v0.47` the proto build tooling must be upgraded. The proto build tooling cannot be upgraded incrementally, it has to be upgraded all at once. To mitigate issues and preserve the QA for both provider and consumer the shared dependencies should be versioned (tagged). Shared dependencies will be moved to a separate `core` module that will be built using the proto tooling intended for its corresponding `cosmos-sdk` version. Initially, we create two `core` module tags: + * `core-45/0.1.0` -> uses legacy build tooling and `cosmos-sdk v0.45` (tag name is subject to change) * `core-47/0.1.0` -> uses new build tooling and `cosmos-sdk v0.47` (tag name is subject to change) @@ -59,16 +68,19 @@ With these changes we will make the shared dependencies available for cosmos-sdk Any work using `cosmos-sdk v0.47` will be uninterrupted by emergency upgrades to older versions. ### QA considerations + We rely on the QA process to assure that provider and consumer are compatible. Breaking the QA will essentially leave us in the dark for a prolonged period of time. We would be doing lots of changes without having a way to reliably ensure that we are not introducing regressions and breaking the core protocol. Besides that, we cannot allow for `main` to be in a broken state for prolonged periods of time because an emergency release would be very difficult to execute. Since emergency releases do happen (as has been shown with dragonberry and changes in ICS regarding consumer chain removals) we cannot simply ignore the systemic risk they introduce. Our workflows must be adapted in a way that accounts for emergency scenarios, to the best of our ability. ### Future prospects + In the future, it is likely that P&C will be locked to the same dependency versions for 90% of the time. However, having the option to remain compatible while performing dependency upgrades will significantly speed up development on both sides. Changes in build tooling without versioning the `core` module would affect both C and P which would preclude the split. ## Steps to perform + The proto build tooling has changed significantly between cosmos-sdk `v0.45` and cosmos-sdk `v0.47`. Due to the fact that `main` currently uses a legacy version of the proto build tool chain, simply upgrading the toolchain breaks both provider and consumer. The changes to proto build system cannot go unaddressed, and they should be addressed first in order to allow smoother development of both provider and consumer. @@ -79,7 +91,6 @@ The changes to proto build system cannot go unaddressed, and they should be addr 4. use `core-v45` in the current implementations of provider and consumer libraries 5. use `core-v47` in the future implementation provider and consumer libraries - ## Decision > This section explains all of the details of the proposed solution, including implementation details. @@ -92,11 +103,15 @@ If the proposed change will be large, please also indicate a way to do the chang > This section describes the consequences, after applying the decision. All consequences should be summarized here, not just the "positive" ones. ### Positive + TODO + ### Negative + TODO ### Neutral + TODO ## References From 86d370700d747d90b4c1ce7faf4276021508a8f4 Mon Sep 17 00:00:00 2001 From: Shawn <44221603+smarshall-spitzbart@users.noreply.github.com> Date: Fri, 28 Apr 2023 15:30:47 -0700 Subject: [PATCH 3/3] content for new idea --- .../adrs/adr-004-separation-of-concerns.md | 81 +++++++------------ 1 file changed, 27 insertions(+), 54 deletions(-) diff --git a/docs/docs/adrs/adr-004-separation-of-concerns.md b/docs/docs/adrs/adr-004-separation-of-concerns.md index e790168133..68b2fd27be 100644 --- a/docs/docs/adrs/adr-004-separation-of-concerns.md +++ b/docs/docs/adrs/adr-004-separation-of-concerns.md @@ -13,83 +13,52 @@ title: Separating consumer and provider libraries > A decision may be "proposed" if it hasn't been agreed upon yet, or "accepted" once it is agreed upon. If a later ADR changes or reverses a decision, it may be marked as "deprecated" or "superseded" with a reference to its replacement. -{Deprecated|Proposed|Accepted} +Proposed ## Context The main deliverable for interchain-security are consumer and provider libraries that are currently located in `x/ccv/provider` and `x/ccv/consumer`. -Any blockchain applications available in the interchain-security repository are used for testing purposes only and should be viewed as examples for building chains implementing ICS. +Any blockchain applications (`app.go`s) available in the interchain-security repository are used for testing purposes only and should be viewed as examples for building chains implementing ICS. By cosmos-sdk terminology and conventions, all applications in the interchain-security repos are just `simapp`s: -* `interchain-security-pd` is a `simapp` for testing the provider ICS library +* `interchain-security-pd` is a `simapp` for testing the provider ICS library * `interchain-security-cd` is a `simapp` for testing the consumer ICS library Any apps existing in the interchain-security repository should not be considered as deliverables as they are not intended to be used by ICS customers, aside from usage in e2e testing frameworks. -Splitting consumer and provider libraries allows different release cycles and makes development easier while maintaining provider and consumer compatibility. -Versioning library code shared between the consumer and provider arises as an intermediary step in the process of splitting the provider and consumer concerns. -By versioning the shared provider and consumer code we benefit from not having to lock the provider and consumer to the same dependencies at all times. +Developers of ICS have found it difficult to reason about semantic versioning within this repository. The main reason for this is that the provider and consumer libraries are not separated. Ie. they share a single `go.mod`, and therefore abide by the same dependencies and release cycle. This means that any change to the provider library affects the consumer library and vice versa. A single release cycle for ICS does not properly communicate if changes were made to the provider library, consumer library, or both. -## Handling shared functionality - -At present provider and consumer libraries share some functionality, type, interfaces and constants. - -Most of the shared funcionality comes from the fact that the provider chain has to communicate the initial validator set to the consumer so that the consumer can include it in its genesis.json (consumer has to know the state of the provider's validator set when the first consumer block is produced). The remainder of shared functionality comprises ICS messaging types and their validation. - -It is important to notice that most of the shared types originate from .proto files. The .proto types are not versioned in `main` which means that any change to .proto types affects both consumer and provider. At present, the consumer and the provider cannot deviate from one another in any way. This is a hinderance and a bottleneck, since work on consumer and provider libraries can be paralelized by introducing a clean split between provider and consumer libraries. - - -### Build tooling incompatibility prevents rapid development - -cosmos-sdk v0.45 and v0.47 use different build tools: - -* https://docs.cosmos.network/v0.47/tooling/protobuf -* https://docs.cosmos.network/v0.47/migrations/upgrading#protobuf +Splitting consumer and provider libraries enables separate release cycles and makes development easier while maintaining provider and consumer compatibility. -`interchain-security` is only half way migrated to the tooling required for `cosmos-sdk v0.47`. Even though ICS has been upgraded to use `buf` the dependency on `gogo/protobuf` remains because `cosmos-sdk v0.45` requires it. Meanwhile, `cosmos-sdk v0.47` has migrated to use [cosmos/gogoproto](https://github.com/cosmos/gogoproto). +## Proposal -Besides that, ICS relies on older build tool images (ghcr.io/cosmos/proto-builder). To be compatible with `cosmos-sdk v0.47` ICS needs to upgrade the proto build image to `ghcr.io/cosmos/proto-builder >= 0.11.5`. +We're proposing the addition of three new `go.mod`s for all of `x/ccv/provider`, `x/ccv/consumer`, `x/ccv/types`. Where the types module does not depend on any other module from ICS, and the provider and consumer modules depend on ONLY the types module. The existing top level `go.mod` will remain in the repo, but will only be used for testing purposes. The top level `go.mod` will naturally depend on all three of the other modules. -### Versioning shared dependencies - -To enable smooth upgrade from cosmos-sdk `v0.45` to `v0.47` the proto build tooling must be upgraded. The proto build tooling cannot be upgraded incrementally, it has to be upgraded all at once. -To mitigate issues and preserve the QA for both provider and consumer the shared dependencies should be versioned (tagged). - -Shared dependencies will be moved to a separate `core` module that will be built using the proto tooling intended for its corresponding `cosmos-sdk` version. - -Initially, we create two `core` module tags: +## Handling shared functionality -* `core-45/0.1.0` -> uses legacy build tooling and `cosmos-sdk v0.45` (tag name is subject to change) -* `core-47/0.1.0` -> uses new build tooling and `cosmos-sdk v0.47` (tag name is subject to change) +At present provider and consumer libraries share some functionality, type, interfaces and constants. -With these changes we will make the shared dependencies available for cosmos-sdk `v0.45` and `v0.47`. This will allow us to add features to ICS versions that still use `cosmos-sdk v0.45` in case we need to perform emergency procedures for ICS versions `<= 1.2.x`. +Most of the shared functionality comes from the fact that the provider chain has to communicate the initial validator set to the consumer so that the consumer can include it in its genesis.json (consumer has to know the state of the provider's validator set when the first consumer block is produced). The remainder of shared functionality comprises ICS messaging types and their validation. -Any work using `cosmos-sdk v0.47` will be uninterrupted by emergency upgrades to older versions. +The shared code between provider and consumers is currently the `x/ccv/types` directory, and should really only be used internally to this repo. In the scenario that code is changed in the `x/ccv/types` directory, that module version can be bumped for internal use, and the provider and consumer libraries can be bumped to use the new version as needed. ### QA considerations -We rely on the QA process to assure that provider and consumer are compatible. Breaking the QA will essentially leave us in the dark for a prolonged period of time. We would be doing lots of changes without having a way to reliably ensure that we are not introducing regressions and breaking the core protocol. +We rely on the QA process to assure that provider and consumer are compatible, and that we have a sane and organized way to isolate patches to an ICS version running in production (whether that be for providers or consumers). -Besides that, we cannot allow for `main` to be in a broken state for prolonged periods of time because an emergency release would be very difficult to execute. Since emergency releases do happen (as has been shown with dragonberry and changes in ICS regarding consumer chain removals) we cannot simply ignore the systemic risk they introduce. Our workflows must be adapted in a way that accounts for emergency scenarios, to the best of our ability. +Since emergency releases do happen (as has been shown with dragonberry and changes in ICS regarding consumer chain removals) we cannot simply ignore the systemic risk they introduce. Our workflows must be adapted in a way that accounts for emergency scenarios, to the best of our ability. -### Future prospects - -In the future, it is likely that P&C will be locked to the same dependency versions for 90% of the time. However, having the option to remain compatible while performing dependency upgrades will significantly speed up development on both sides. - -Changes in build tooling without versioning the `core` module would affect both C and P which would preclude the split. +We believe that separating provider and consumer versioning with separated `go.mod`s is the best way to easily reason about what feature/fixes affect consumers, providers, or both. ## Steps to perform -The proto build tooling has changed significantly between cosmos-sdk `v0.45` and cosmos-sdk `v0.47`. Due to the fact that `main` currently uses a legacy version of the proto build tool chain, simply upgrading the toolchain breaks both provider and consumer. +Do some refactors, but keep x/ccv as minimally changed as possible. -The changes to proto build system cannot go unaddressed, and they should be addressed first in order to allow smoother development of both provider and consumer. - -1. separate out the `core` module that comprises types, functions and interfaces shared by provider and consumer -2. version (tag) the `core-v45` module using legacy proto build tooling (`cosmos-sdk v0.45`, `gogo/protobuf`, `ghcr.io/cosmos/proto-builder >= 0.9.0`) -3. version (tag) the `core-v47` module using latest protobuild tooling (`cosmos-sdk v0.47`, `cosmos/gogoproto`, `ghcr.io/cosmos/proto-builder >= 0.11.5`) -4. use `core-v45` in the current implementations of provider and consumer libraries -5. use `core-v47` in the future implementation provider and consumer libraries +1. Refactor `x/ccv/types` s.t. it doesn't have package dependencies from this repo. Make the types folder the lowest level of dependency. Make sure `x/ccv/types` only contains code that is shared by consumer/provider (including .pb files, this will require some changes to .proto files). +2. Refactor `x/ccv/provider` and `x/ccv/consumer` s.t. they only rely on `x/ccv/types`, not one another, and not any other packages from this repo (not even `testutils/` for example). I've confirmed this is possible. +3. Add three new `go.mod`s for all of `x/ccv/provider`, `x/ccv/consumer`, `x/ccv/types`, module import dependencies match what's described in previous steps. +4. Split out `make proto-gen` into `make proto-gen-provider`, `make proto-gen-consumer`, and `make proto-gen-types`. This is necessary since `make proto-gen` would be responsible for generating .pb files to 3 different submodules, each of which could require different protobuf build tooling (due to reliance on sdk 45 vs sdk 47 for example). ## Decision @@ -98,24 +67,28 @@ It should also describe affects / corollary items that may need to be changed as If the proposed change will be large, please also indicate a way to do the change to maximize ease of review. (e.g. the optimal split of things to do between separate PR's) +Decision not yet reached + ## Consequences > This section describes the consequences, after applying the decision. All consequences should be summarized here, not just the "positive" ones. ### Positive -TODO +* Less refactoring than initial idea. +* We will rarely need to release new versions for `x/ccv/types`, and if we do, it would truly be a change that is relevant to both consumer and provider. A version bump to `x/ccv/types` does not necessarily mean both consumer and provider must immediately upgrade to the new version tho, take sdk upgrade as example. +* The association between changes in `x/ccv` and whether this affects consumer/provider is very clear. Semantic versioning for consumer and provider actually means something. Emergency patches to specific versions of consumer/provider module are easier to execute and keep track of. ### Negative -TODO +* More work having to do .pb file generation compared to initial idea. +* There are situations where certain branches of ICS would not be able to execute one or multiple of the 3 make proto-gen-______ options without breaking code. ### Neutral -TODO ## References > Are there any relevant PR comments, issues that led up to this, or articles referrenced for why we made the given design choice? If so link them here! -* {reference link} +* https://github.com/cosmos/interchain-security/pull/890 was a SPIKE that inspired the ideas in this document.