feat(schema): add API descriptors, struct, oneof & list types, and wire encoding spec #21482
Conversation
|
Warning Rate limit exceeded@tac0turtle has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 14 minutes and 57 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 Walkthrough📝 WalkthroughWalkthroughThe changes introduce a structured API descriptor system, enhancing compatibility management for APIs. New types and fields are defined across several files, including Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant Descriptor
Client->>API: Request API Information
API->>Descriptor: Fetch APIDescriptor
Descriptor-->>API: Return APIDescriptor
API-->>Client: Provide API Information
sequenceDiagram
participant Client
participant OneOfType
participant OneOfCase
Client->>OneOfType: Define OneOf Type
OneOfType->>OneOfCase: Add Case
OneOfCase-->>OneOfType: Case Added
OneOfType-->>Client: OneOf Type Defined
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
@aaronc your pull request is missing a changelog! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (6)
- schema/api.go (1 hunks)
- schema/field.go (1 hunks)
- schema/kind.go (4 hunks)
- schema/object_type.go (1 hunks)
- schema/oneof.go (1 hunks)
- schema/struct.go (1 hunks)
Additional context used
Path-based instructions (6)
schema/struct.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.schema/oneof.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.schema/field.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.schema/object_type.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.schema/api.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.schema/kind.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (24)
schema/struct.go (3)
1-2: LGTM!The package declaration is correct and no imports are needed at this point.
3-21: LGTM!The
StructTypedefinition and its documentation comments are well-written and informative. The comments clearly mention the current implementation status and provide a good explanation of the type's purpose and the usage of its fields. The doc comments follow the Golang conventions.
1-21: Code conforms to the Uber Golang style guide.The code follows the Uber Golang style guide for naming conventions, doc comments, and formatting. There are no deviations from the style guide.
schema/oneof.go (3)
37-43: LGTM!The
OneOfValuedefinition looks good and is straightforward.
5-19: LGTM!The
OneOfTypedefinition looks good. The comments provide clear instructions on compatible and incompatible changes.Verify the allowed values for
DiscriminantKind.Ensure that the code that sets the
DiscriminantKindfield only allows the values mentioned in the comment:Uint8Kind,Int8Kind,Uint16Kind,Int16Kind, orInt32Kind.Run the following script to verify the allowed values:
Verification successful
No issues found with
DiscriminantKindusage.The
DiscriminantKindfield is defined but not currently used or assigned in the codebase. There are no issues with the allowed values since it is not being utilized. If this field is intended for future use, ensure that only the specified values are allowed when it is implemented.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the allowed values for `DiscriminantKind`. # Test: Search for assignments to `DiscriminantKind`. Expect: Only allowed values. rg --type go -A 5 $'DiscriminantKind ='Length of output: 40
Script:
#!/bin/bash # Description: Search for the usage of `DiscriminantKind` in a broader context. # Search for `DiscriminantKind` in struct initializations and function parameters. rg --type go 'DiscriminantKind'Length of output: 146
22-34: LGTM!The
OneOfCasedefinition looks good. The comments provide clear instructions on the fields.Verify the restriction on the
Kindfield.Ensure that the code that sets the
Kindfield does not allowListKind.Run the following script to verify the restriction:
schema/field.go (1)
18-18: LGTM!The updated comment provides more clarity on the usage of the
ReferencedTypefield.schema/object_type.go (2)
12-19: LGTM!The updated comments for the
KeyFieldsattribute provide clearer guidance on the constraints and implications of modifying the attribute. The changes are approved.
25-28: LGTM!The updated comments for the
ValueFieldsattribute provide clearer guidance on the constraints and implications of modifying the attribute. The changes are approved.schema/api.go (3)
41-50: LGTM!The
APIDescriptortype is well-defined and the comments provide clear guidelines on compatibility and encoding.
53-85: LGTM!The
MethodDescriptortype is well-defined and the comments provide clear guidelines on compatibility and encoding.
88-98: LGTM!The
Volatilitytype and its constants are well-defined and the comments provide clear explanations.schema/kind.go (12)
13-31: Documentation changes look great!The expanded documentation for the
Kindtype provides crucial encoding specifications for each kind, enhancing clarity and completeness. This will help developers properly encode and decode data in various formats.
41-44:StringKindencoding documentation looks good!The expanded documentation for
StringKindprovides clear specifications for key binary and value binary encodings. The distinction between non-terminal and terminal forms in the key binary encoding is important. Reusing theBytesKindvalue binary encoding promotes consistency.
51-58:BytesKindencoding documentation looks good!The expanded documentation for
BytesKindprovides clear specifications for key binary and value binary encodings. The use of length prefixes in the non-terminal key binary encoding allows for proper decoding. The value binary encoding using offset and length integers provides flexibility for referencing the actual bytes.
64-65: Numeric kind encoding documentation looks good!The expanded documentation for the numeric kinds (
Int8Kind,Uint8Kind,Int16Kind,Uint16Kind,Int32Kind,Uint32Kind,Int64Kind,Uint64Kind) provides clear and consistent specifications for key binary and value binary encodings.The use of big-endian encoding for keys allows for proper sorting. The first bit inversion technique for signed integer keys is clever for maintaining sort order. The little-endian encoding for values is a common choice for numeric data.
Also applies to: 71-72, 78-79, 85-86, 92-93, 99-100, 107-108, 115-116
123-124:IntegerStringKindandDecimalStringKindJSON encoding documentation looks good!The expanded documentation for
IntegerStringKindandDecimalStringKindprovides clear specifications for JSON encoding using base10 integer and decimal strings, respectively. The use ofIntegerFormatandDecimalFormatregexes ensures format conformance.The canonical encoding rules, such as no leading zeros for integers and specific exponential notation rules for decimals, promote consistency and ease of parsing.
Also applies to: 133-134
140-141:BoolKindencoding documentation looks good!The expanded documentation for
BoolKindprovides a clear and concise specification for key binary and value binary encodings. The use of a single byte to represent boolean values, with 0 for false and 1 for true, is a common and efficient approach.
151-152:TimeKindandDurationKindencoding documentation looks good!The expanded documentation for
TimeKindandDurationKindprovides clear specifications for JSON, key binary, and value binary encodings.For JSON encoding, the use of ISO 8601 timestamp strings for
TimeKindensures interoperability and ease of parsing. The canonical encoding rules promote consistency. TheDurationKindJSON encoding using decimal strings with 's' suffix is straightforward.The binary encoding specifications are consistent with the encoding of other numeric kinds, ensuring proper sorting and efficient representation.
Also applies to: 160-161
167-168:Float32KindandFloat64Kindencoding documentation looks good!The expanded documentation for
Float32KindandFloat64Kindprovides clear specifications for key binary and value binary encodings. The use of IEEE-754 encoding, with 4 bytes forFloat32Kindand 8 bytes forFloat64Kind, is the standard representation for floating-point numbers, ensuring interoperability and precision.Also applies to: 174-175
185-188:AddressKindencoding documentation looks good!The expanded documentation for
AddressKindprovides clear specifications for key binary and value binary encodings. The use of a 1-byte length prefix allows for efficient encoding and decoding of variable-length addresses. The distinction between non-terminal and terminal forms in the key binary encoding is important for proper encoding.The maximum supported length of 63 bytes for addresses provides a reasonable limit for address sizes.
196-197:EnumKindencoding documentation looks good!The expanded documentation for
EnumKindprovides a clear specification for key binary and value binary encodings. By using the same binary encoding as theEnumType's numeric kind, it ensures consistency and compatibility with the underlying enum representation.
203-204:JSONKindencoding documentation looks good!The expanded documentation for
JSONKindprovides a clear specification for key binary and value binary encodings. Using string encoding for both key binary and value binary encodings is a straightforward and interoperable approach for representing JSON data.
206-216:UIntNKindaddition looks good, but clarification needed on implementation timeline.The addition of
UIntNKindprovides flexibility for representing unsigned integers with arbitrary bit widths. The documentation clearly specifies the encoding formats for Go, JSON, and binary representations, which is consistent with the encoding schemes used for other numeric kinds.However, the documentation mentions that support for
UIntNKindis currently unimplemented.Can you please clarify the timeline for implementing support for
UIntNKind? It would be helpful to know when this kind will be usable in practice.
| // ElementKind is the element type when Kind is ListKind. | ||
| // Support for this is currently UNIMPLEMENTED, this notice will be removed when it is added. | ||
| ElementKind Kind `json:"element_kind,omitempty"` |
There was a problem hiding this comment.
Add implementation for the ElementKind field.
The ElementKind field seems to be a useful addition for representing the element type of a list. However, the implementation is currently missing.
Please add the implementation for the ElementKind field and remove the notice about the missing implementation from the comment. Let me know if you need any help with the implementation.
| // Size specifies the size or max-size of a field. | ||
| // Support for this is currently UNIMPLEMENTED, this notice will be removed when it is added. | ||
| // Its specific meaning may vary depending on the field kind. | ||
| // For IntNKind and UintNKind fields, it specifies the bit width of the field. | ||
| // For StringKind, BytesKind, AddressKind, and JSONKind, fields it specifies the maximum length rather than a fixed length. | ||
| // If it is 0, such fields have no maximum length. | ||
| // It is invalid to have a non-zero Size for other kinds. | ||
| Size uint32 `json:"size,omitempty"` |
There was a problem hiding this comment.
Add implementation for the Size field.
The Size field seems to be a useful addition for specifying the size or max-size of a field. The comment provides a clear explanation of the field's usage for different field kinds. However, the implementation is currently missing.
Please add the implementation for the Size field and remove the notice about the missing implementation from the comment. Let me know if you need any help with the implementation.
|
|
||
| // OutputFields is the list of output fields for the method. | ||
| // | ||
| // It is a COMPATIBLE change to add new output fields to a method, |
There was a problem hiding this comment.
I think for now we should amend this until we have strong guarantees that it cannot be consensus breaking.
* main: docs: amend docs for 52 changes (#21992) test: migrate e2e/authz to system tests (#21819) refactor(runtime/v2): use StoreBuilder (#21989) feat(schema): add API descriptors, struct, oneof & list types, and wire encoding spec (#21482) docs: add instructions to change DefaultGenesis (#21680) feat(x/staking)!: Add metadata field to validator info (#21315) chore(x/authz)!: Remove account keeper dependency (#21632)
* main: docs: amend docs for 52 changes (#21992) test: migrate e2e/authz to system tests (#21819) refactor(runtime/v2): use StoreBuilder (#21989) feat(schema): add API descriptors, struct, oneof & list types, and wire encoding spec (#21482) docs: add instructions to change DefaultGenesis (#21680) feat(x/staking)!: Add metadata field to validator info (#21315) chore(x/authz)!: Remove account keeper dependency (#21632) chore(contributing): delete link (#21990) test(gov): Migrate e2e to system test (#21927) test: e2e/client to system tests (#21981)
Description
This PR is defines additional kinds and types in
cosmossdk.io/schemawithout implementing support yet:UIntNandIntNfor larger fixed width integersDeterministic binary key and value encoding formats are also specified.
This PR is intended as a code-based RFC/ADR on eventual features of schema.
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
Documentation