-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor(codec/unknownproto): use official descriptorpb types, do not rely on gogo code gen types. #18541
Conversation
WalkthroughWalkthroughThe changes involve refactoring the Changes
Assessment against linked issues
Related issues (Beta)
TipsChat with CodeRabbit Bot (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- codec/unknownproto/unknown_fields.go (10 hunks)
Additional comments: 8
codec/unknownproto/unknown_fields.go (8)
10-19: The import of
github.com/cosmos/gogoproto/proto
should be removed if it is no longer used after the refactoring to usedescriptorpb
types.100-113: The comment on lines 111-113 references
github.com/cosmos/gogoproto
, which may no longer be relevant if the dependency ongogo
protobuf has been removed. Verify if this comment needs updating or removal.198-257: The
checks
array has been updated to usedescriptorpb.FieldDescriptorProto_Type
. Ensure that all references tochecks
throughout the codebase are updated to use the correct index type (int
instead ofprotowire.Type
).260-265: The
canEncodeType
function signature has been updated to useint
forwireType
. Ensure that all calls to this function are updated accordingly.364-370: The
extractFileDescMessageDesc
function is usingprotov2.Unmarshal
which is correct as per the refactor. However, ensure that theprotoBlob
is correctly formatted fordescriptorpb.FileDescriptorProto
before unmarshalling.381-384: The
descriptorMatch
struct has been updated to usedescriptorpb.FieldDescriptorProto
. Ensure that all usages ofdescriptorMatch
are consistent with this change.440-483: The
findWireTypeFromFieldDescriptorProtoType
function should be reviewed to ensure that it returns the correctprotowire.Type
for eachdescriptorpb.FieldDescriptorProto_Type
. This is critical as the function is used to determine the expected wire type for a given field descriptor type.486-509: The
isScalar
function checks if a field is a scalar type. Ensure that the function's logic is still correct after the refactor and that it aligns with the updateddescriptorpb.FieldDescriptorProto_Type
values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- CHANGELOG.md (1 hunks)
Additional comments: 1
CHANGELOG.md (1)
- 67-67:
The changelog entry is clear and follows the repository's format for documenting changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (1)
- codec/unknownproto/unknown_fields.go (10 hunks)
Additional comments: 7
codec/unknownproto/unknown_fields.go (7)
13-17: The import of
github.com/cosmos/gogoproto/proto
should be removed if it's no longer used after the refactor to usedescriptorpb
.101-114: The comment on lines 112-113 seems to be outdated and should be updated or removed if it no longer applies to the current implementation.
199-258: The
checks
array is a good optimization for constant time lookups. Ensure that all wire types and descriptor types are correctly mapped and that no types are missing.365-371: Ensure that the error from
gzip.NewReader
andio.ReadAll
are handled properly and that the resources are closed appropriately to avoid resource leaks.380-385: The
descriptorMatch
struct is used for caching, ensure that the cache is properly synchronized to prevent data races.407-413: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [392-413]
When caching descriptor information in
descprotoCache
, ensure that the cache is invalidated or updated when descriptors change to prevent stale data issues.
- 487-510: The
isScalar
function correctly identifies scalar types. Ensure that this function is used consistently throughout the codebase to determine scalar types.
codec/unknownproto/unknown_fields.go
Outdated
Type: reflect.ValueOf(msg).Type().String(), | ||
TagNum: tagNum, | ||
GotWireType: wireType, | ||
WantWireType: protowire.Type(fieldDescProto.WireType()), | ||
WantWireType: findWireTypeFromFieldDescriptorProtoType(fieldDescProto.GetType()), | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error struct errMismatchedWireType
should include the expected and actual wire types using descriptorpb.FieldDescriptorProto_Type
instead of protowire.Type
to align with the refactor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- CHANGELOG.md (1 hunks)
Additional comments: 1
CHANGELOG.md (1)
- 68-68: The changelog has been updated correctly to reflect the changes made in pull requests refactor(codec/unknownproto): use official descriptorpb types, do not rely on gogo code gen types. #18541 and imp(staking): detect the length of the ed25519 pubkey in CreateValidator to prevent panic #18506.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this change @testinginprod. I've added some comments. Although what's the rationale behind this change? Is it to get rid of the protogen-gogo-descriptor package? If it is, I believe we forked an entire repository for which it'll still exist. Thank you
@odeke-em thank you for the review. This is a preliminary step PR to make a more significant change to unknownproto, specifically unknonwproto currently does not allow This is not desirable for two reasons:
Alongside enabling support for having non-interface-registry anys in TXs, we want to also be able to decode anys which have a protov2 counterparty but not a gogoproto one. Using default descriptorpb types makes this transition a lot more effortless otherwise we'd be forced to convert from gogo-protoc-gen.Descriptor into descriptorp.Descriptor . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- codec/unknownproto/unknown_fields.go (10 hunks)
Additional comments: 11
codec/unknownproto/unknown_fields.go (11)
12-20: The import changes align with the summary and the refactoring goal of using
descriptorpb
types from the official protobuf library.69-72: The update to use
fieldTypeToProtowireType
for determiningWantWireType
is consistent with the refactoring goal and should ensure correct wire type matching.104-104: The update to use
isScalar
for checking if a field is scalar aligns with the refactoring goal and the summary.199-266: The refactoring of
checks
to usedescriptorpb.FieldDescriptorProto_Type
is consistent with the goal of usingdescriptorpb
types.333-335: The update of the global variable
protoFileToDesc
to usedescriptorpb.FileDescriptorProto
is consistent with the refactoring goal.337-342: The update of the
unnestDesc
function signature to usedescriptorpb.DescriptorProto
is consistent with the refactoring goal.347-350: The update of the
extractFileDescMessageDesc
function signature to usedescriptorpb
types is consistent with the refactoring goal.393-396: The update of the
getDescriptorInfo
function signature to usedescriptorpb
types is consistent with the refactoring goal.441-442: The
DefaultAnyResolver.Resolve
function signature appears unchanged, which seems to be a no-op. Please verify if this was intentional or if changes were expected but not made.445-504: The addition of the
fieldTypeToProtowireType
andisScalar
functions is consistent with the refactoring goal and the summary.480-481: The citation for the
isScalar
function is correctly included, addressing the previous review comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (2)
- CHANGELOG.md (1 hunks)
- codec/unknownproto/unknown_fields.go (10 hunks)
Additional comments: 14
codec/unknownproto/unknown_fields.go (14)
12-20: The import changes align with the summary, replacing
github.com/cosmos/gogoproto/protoc-gen-gogo/descriptor
withprotov2 "google.golang.org/protobuf/proto"
and"google.golang.org/protobuf/types/descriptorpb"
.69-72: The use of
fieldTypeToProtowireType
in theerrMismatchedWireType
struct instantiation is consistent with the summary.104-104: The use of
isScalar
in the condition withinRejectUnknownFields
is consistent with the summary.199-258: The constants and maps have been correctly updated to use
descriptorpb.FieldDescriptorProto
.199-258: The
protoMessageForTypeName
function has been updated to return an error, aligning with the summary.261-266: The
canEncodeType
function has been updated to takedescriptorpb.FieldDescriptorProto_Type
, which is consistent with the summary.337-342: The
unnestDesc
function has been updated to takedescriptorpb.DescriptorProto
, which is consistent with the summary.365-372: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [347-372]
The
extractFileDescMessageDesc
function has been updated to returndescriptorpb.FileDescriptorProto
anddescriptorpb.DescriptorProto
, which is consistent with the summary.
383-384: The
cache
field in thedescriptorMatch
struct has been updated to usedescriptorpb.FieldDescriptorProto
, which is consistent with the summary.393-396: The
getDescriptorInfo
function has been updated to returnmap[int32]*descriptorpb.FieldDescriptorProto
and*descriptorpb.DescriptorProto
, which is consistent with the summary.333-334: The
protoFileToDesc
map has been updated to usedescriptorpb.FileDescriptorProto
, which is consistent with the summary.441-442: The
Resolve
method inDefaultAnyResolver
has been updated to returnproto.Message
and an error, which is consistent with the summary.445-477: The new function
fieldTypeToProtowireType()
has been added, aligning with the summary.479-505: The new function
isScalar()
has been added, aligning with the summary. However, the comment indicates that the code was copied fromgogo/protobuf
, which matches the existing comment from a previous review. This should be verified to ensure proper attribution is given.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @testinginprod, LGTM with some comments.
thank you @odeke-em, reviews have been addressed. |
Awesome, thank you and LGTM++ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- codec/unknownproto/unknown_fields.go (10 hunks)
Additional comments: 14
codec/unknownproto/unknown_fields.go (14)
13-17: The import changes are consistent with the summary and reflect the transition to official
descriptorpb
types.69-72: The replacement of
protowire.Type(fieldDescProto.WireType())
withtoProtowireType(fieldDescProto.GetType())
is consistent with the summary.104-104: The replacement of
fieldDescProto.IsScalar()
withisScalar(fieldDescProto)
is consistent with the summary.202-258: The constants and maps have been correctly updated to use
descriptorpb.FieldDescriptorProto
.263-266: The function signature for
canEncodeType
has been correctly updated to usedescriptorpb.FieldDescriptorProto_Type
.337-342: The function signature for
unnestDesc
has been correctly updated to usedescriptorpb.DescriptorProto
.365-372: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [347-372]
The function
extractFileDescMessageDesc
has been correctly updated to returndescriptorpb
types.
382-384: The type
descriptorMatch
has been correctly updated to usedescriptorpb
types.407-413: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [392-413]
The function
getDescriptorInfo
has been correctly updated to returndescriptorpb
types.
333-333: The map
protoFileToDesc
has been correctly updated to usedescriptorpb.FileDescriptorProto
.445-480: The function
toProtowireType
has been correctly added to the codebase.485-507: The function
isScalar
has been correctly added to the codebase.484-484: The citation for the
isScalar
function has been added as requested in a previous review.459-459: The code has appropriate spacing between sections, which should aid in readability as suggested in a previous review.
@testinginprod please update with the latest from main. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- codec/unknownproto/unknown_fields.go (10 hunks)
Additional comments: 12
codec/unknownproto/unknown_fields.go (12)
13-17: The import changes are correctly reflected in the code, aligning with the summary.
69-74: The update to use
toProtowireType
in theRejectUnknownFields
function is correctly implemented.105-115: The update to use
isScalar
in theRejectUnknownFields
function is correctly implemented.200-267: The constants and maps related to field types have been updated correctly, reflecting the changes from
descriptor.FieldDescriptorProto_Type
todescriptorpb.FieldDescriptorProto_Type
.264-267: The function
canEncodeType
has been correctly updated to takedescType descriptorpb.FieldDescriptorProto_Type
.334-345: The map
protoFileToDesc
has been correctly updated to usedescriptorpb.FileDescriptorProto
.338-343: The function
unnestDesc
has been correctly updated to takemdescs []*descriptorpb.DescriptorProto
.366-373: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [348-373]
The function
extractFileDescMessageDesc
has been correctly updated to return*descriptorpb.FileDescriptorProto
and*descriptorpb.DescriptorProto
.
383-385: The type
descriptorMatch
has been correctly updated to use*descriptorpb.FieldDescriptorProto
and*descriptorpb.DescriptorProto
.394-397: The function
getDescriptorInfo
has been correctly updated to returnmap[int32]*descriptorpb.FieldDescriptorProto
and*descriptorpb.DescriptorProto
.447-480: The function
toProtowireType
has been correctly added to convertdescriptorpb.FieldDescriptorProto_Type
toprotowire.Type
.486-508: The function
isScalar
has been correctly added to determine whether a field is a scalar type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
Description
Closes: #18540
This PR removes de gogo-proto-gen dependency from unknown proto and uses the official descriptor APIs living google.golang.org/protobuf.
This is a required step for a sub-sequent PR which will enable the usage of google.Protobuf.Anys in Txs without them being registered in the interface registry.
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...
!
to the type prefix if API or client breaking changeCHANGELOG.md
make lint
andmake test
Reviewers 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.
I have...
!
in the type prefix if API or client breaking changeSummary by CodeRabbit
Refactor
descriptorpb
types for enhanced compatibility and future maintenance.Bug Fixes
New Features
Documentation
descriptorpb
types and the removal of deprecatedprotoc-gen-gogo/descriptor
usage.