feat: return timestamp from GetIDs
#103
Conversation
WalkthroughThe changes in this pull request primarily focus on enhancing the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (2)
Files skipped from review as they are similar to previous changes (1)
Additional context usedLearnings (1)
Additional comments not posted (1)
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
CodeRabbit Configuration File (
|
RollkitBot
requested review from
a team,
Manav-Aggarwal,
gupadhyaya and
tuxcanfly
and removed request for
a team
tzdybal
added
enhancement
|
could you fix golangci-lint? otherwise, LGTM |
tzdybal
force-pushed
the
tzdybal/timestamp
branch
from
7372412 to
456872e
Compare
Enhanced the GetIDs functionality to include a timestamp in the response, both in the proto files and Go structures. Updated relevant files and added checks for the new field in the tests.
tzdybal
force-pushed
the
tzdybal/timestamp
branch
7 times, most recently
from
0ae50c1 to
db276af
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
proxy/grpc/client.go (1)
69-80: Approve the changes to theGetIDsmethod.The changes to the
GetIDsmethod look good:
- The return type has been updated to
*da.GetIDsResultto include the timestamp along with the IDs, aligning with the PR objective.- The timestamp is correctly extracted from the response using
types.TimestampFromProto.- The new
da.GetIDsResultstruct is a good choice for returning the IDs and timestamp, as it makes the return values more explicit.- Error handling for the timestamp conversion is properly implemented.
Note that this is a breaking change due to the modified method signature. Ensure that all callers of this method are updated to handle the new return type.
The addition of the timestamp provides valuable context to the caller about when the IDs were retrieved. This extra information can be beneficial for debugging, logging, and understanding the state of the system at a given point in time.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (3)
buf.lockis excluded by!**/*.lockgo.sumis excluded by!**/*.sumtypes/pb/da/da.pb.gois excluded by!**/*.pb.go
Files selected for processing (13)
- Makefile (1 hunks)
- buf.gen.yaml (1 hunks)
- buf.yaml (1 hunks)
- da.go (3 hunks)
- go.mod (3 hunks)
- proto/da/da.proto (2 hunks)
- proto/gen.sh (0 hunks)
- proto/protoc.sh (0 hunks)
- proxy/grpc/client.go (2 hunks)
- proxy/grpc/server.go (2 hunks)
- proxy/jsonrpc/client.go (2 hunks)
- test/dummy.go (6 hunks)
- test/test_suite.go (2 hunks)
Files not reviewed due to no reviewable changes (2)
- proto/gen.sh
- proto/protoc.sh
Files skipped from review as they are similar to previous changes (5)
- da.go
- proto/da/da.proto
- proxy/jsonrpc/client.go
- test/dummy.go
- test/test_suite.go
Additional comments not posted (12)
buf.yaml (2)
2-3: LGTM!The addition of the
buf.build/cosmos/gogo-protodependency is a common practice in projects that utilize Protobuf and require Golang-specific features. It is a well-known and widely-used package in the Cosmos ecosystem.
10-10: LGTM!The change from
DEFAULTtoSTANDARDlinting rule is a good practice to enforce stricter coding standards and improve code quality. It demonstrates the project's commitment to maintaining a clean and consistent codebase.buf.gen.yaml (1)
6-15: LGTM! The changes enhance the code generation process.The shift to the
gocosmosplugin and the expanded type mappings offer several potential benefits:
- Improved integration with the Cosmos ecosystem.
- Better type handling in the generated code.
- More granular control over the code generation process.
The retention of gRPC-related settings ensures that the core functionality remains intact.
Overall, these changes reflect a significant update in the code generation process while maintaining compatibility and potentially improving the generated code quality.
go.mod (3)
6-6: LGTM!The addition of
github.com/cosmos/gogoprotoat versionv1.7.0aligns with the PR objectives and ensures a stable version of the library is used.
16-16: LGTM!The addition of
github.com/google/go-cmpat versionv0.6.0as an indirect dependency is acceptable. It ensures a stable version of the library is used by the module's dependencies.
30-30: LGTM!The update of
google.golang.org/protobufto versionv1.34.2is a minor version bump that should include bug fixes and improvements while maintaining backward compatibility.Makefile (3)
68-74: LGTM!The new
check-proto-depstarget is a good addition to ensure that the necessary protobuf dependency is installed before running theproto-genandproto-linttargets. This improves the reliability of the build process.
76-80: LGTM!The modifications to the
proto-gentarget improve the robustness of the protobuf file generation process. Using thebuftool provides a more standardized and maintainable approach compared to custom scripts. The dependency oncheck-proto-depsensures that the necessary protobuf dependency is installed before running the target.
83-87: LGTM!The modifications to the
proto-linttarget improve the robustness of the protobuf file linting process. Using thebuftool provides a more standardized and maintainable approach compared to custom scripts. The dependency oncheck-proto-depsensures that the necessary protobuf dependency is installed before running the target.proxy/grpc/server.go (2)
6-6: LGTM!The import statement is valid and does not raise any concerns. The package is likely used in the subsequent code changes.
42-51: Excellent work!The code segment correctly implements the required changes to include the timestamp in the
GetIdsresponse. It appropriately captures the response fromp.target.GetIDs, converts the timestamp usingtypes.TimestampProto, handles the potential error, and returns apbda.GetIdsResponsewith the converted timestamp and IDs. The changes align with the method signature and the expected behavior.proxy/grpc/client.go (1)
6-6: LGTM!The import statement for the
github.com/cosmos/gogoproto/typespackage has been added correctly. This package is commonly used when working with protocol buffer types in Go.
tzdybal
force-pushed
the
tzdybal/timestamp
branch
3 times, most recently
from
ad36445 to
f32a51d
Compare
tzdybal
force-pushed
the
tzdybal/timestamp
branch
from
f32a51d to
bd91110
Compare
Removed old protobuf generation shell scripts and updated Makefile to use Buf. Changed dependencies and updated configurations accordingly. This modernizes the setup and removes the dependency on Docker.
Replaced the int64 timestamp with google.protobuf.Timestamp in the GetIdsResponse message. Adjusted the generated code to handle the new timestamp type and updated the protobuf imports accordingly.
tzdybal
force-pushed
the
tzdybal/timestamp
branch
from
bd91110 to
c82ed55
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (3)
buf.lockis excluded by!**/*.lockgo.sumis excluded by!**/*.sumtypes/pb/da/da.pb.gois excluded by!**/*.pb.go
Files selected for processing (11)
- .github/workflows/lint.yml (1 hunks)
- .github/workflows/proto.yml (1 hunks)
- Makefile (1 hunks)
- buf.gen.yaml (1 hunks)
- buf.yaml (1 hunks)
- go.mod (3 hunks)
- proto/da/da.proto (2 hunks)
- proto/gen.sh (0 hunks)
- proto/protoc.sh (0 hunks)
- proxy/grpc/client.go (2 hunks)
- proxy/grpc/server.go (2 hunks)
Files not reviewed due to no reviewable changes (2)
- proto/gen.sh
- proto/protoc.sh
Files skipped from review as they are similar to previous changes (5)
- buf.gen.yaml
- buf.yaml
- go.mod
- proto/da/da.proto
- proxy/grpc/server.go
Additional context used
actionlint
.github/workflows/proto.yml
11-11: shellcheck reported issue in this script: SC2086:info:1:36: Double quote to prevent globbing and word splitting
(shellcheck)
.github/workflows/lint.yml
55-55: shellcheck reported issue in this script: SC2086:info:1:36: Double quote to prevent globbing and word splitting
(shellcheck)
Additional comments not posted (6)
Makefile (3)
68-73: LGTM!The addition of the
check-proto-depstarget is a good practice to ensure that the necessary dependencies are present before running theproto-genandproto-linttargets. This helps maintain a reliable build environment.
75-78: LGTM!The modifications to the
proto-gentarget are beneficial:
- Depending on the
check-proto-depstarget ensures that the necessary dependencies are present before generating the protobuf files.- Updating the command to use the
buftool enhances the efficiency and reliability of the protobuf file generation process.
81-84: LGTM!The modifications to the
proto-linttarget are beneficial:
- Depending on the
check-proto-depstarget ensures that the necessary dependencies are present before linting the protobuf files.- Updating the command to use the
buftool enhances the efficiency and reliability of the protobuf file linting process.proxy/grpc/client.go (3)
6-6: LGTM!The import statement for the
typespackage fromgit.colasdn.top/cosmos/gogoprotois necessary to support the changes made in theGetIDsmethod for working with protocol buffer timestamp types.
Line range hint
1-68: Skipped reviewing unchanged code.The remaining code in the file is unchanged and does not require further review.
Also applies to: 81-165
69-80: Excellent work on enhancing theGetIDsmethod!The changes made to the
GetIDsmethod align perfectly with the PR objective to return a timestamp along with the IDs. Key highlights:
- The method signature has been appropriately modified to return a pointer to
da.GetIDsResult, which is a struct containing the IDs and the timestamp.- The timestamp is correctly extracted from the response using
types.TimestampFromProtoand any error during the conversion is properly handled.- Returning a struct instead of a tuple enhances the readability and maintainability of the code.
Please ensure that all callers of the
GetIDsmethod are updated to handle the new return type. You can use the following script to find the callers:Verification successful
GetIDs method changes have been consistently implemented
The modifications to the
GetIDsmethod have been successfully and consistently implemented across the codebase. Key observations:
- The method signature returning
(*da.GetIDsResult, error)is uniform across all implementations (da.go, test/dummy.go, proxy/jsonrpc/client.go, proxy/grpc/client.go).- Test cases in test/test_suite.go have been updated to use the new signature.
- The gRPC server (proxy/grpc/server.go) and JSON-RPC client (proxy/jsonrpc/client.go) are correctly using the new
GetIDsmethod signature.These findings confirm that the changes have been properly propagated throughout the codebase, and all callers have been updated to handle the new return type (struct containing IDs and timestamp).
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all callers of the `GetIDs` method. # Test: Search for the method usage. Expect: List of files containing the method calls. rg --type go -w $'\\.\\.GetIDs\\('Length of output: 32
Script:
#!/bin/bash # Description: Find potential callers of the GetIDs method and verify its definition # Search for potential GetIDs method calls with a more lenient pattern echo "Searching for potential GetIDs method calls:" rg --type go 'GetIDs\(' # Search for the GetIDs method definition echo -e "\nSearching for GetIDs method definition:" rg --type go 'func.*GetIDs\(' # If we still can't find anything, search for any occurrence of 'GetIDs' echo -e "\nSearching for any occurrence of 'GetIDs':" rg --type go 'GetIDs'Length of output: 3346
MSevey
left a comment
MSevey
left a comment
There was a problem hiding this comment.
I think the edits to the workflow files actually just need https://github.com/actions/setup-go
tzdybal
force-pushed
the
tzdybal/timestamp
branch
2 times, most recently
from
af30fa1 to
e113a6a
Compare
tzdybal
force-pushed
the
tzdybal/timestamp
branch
from
e113a6a to
49f54f5
Compare
MSevey
left a comment
MSevey
left a comment
There was a problem hiding this comment.
One more improvement we might as well make since we are touching the CI files
Overview
Enhanced the GetIDs functionality to include a timestamp in the response, both in the proto files and Go structures. Updated relevant files and added checks for the new field in the tests.
Resolves #101
Summary by CodeRabbit
New Features
GetIDsResultstructure that includes both IDs and a timestamp, enhancing the context of retrieved data.timestampfield to theGetIdsResponsemessage for improved temporal context.check-proto-depstarget to streamline protobuf dependency management.gocosmosfor better compatibility with the Cosmos ecosystem.buf.build/cosmos/gogo-prototo enhance protobuf processing.Bug Fixes
Chores