Update to newest protos #422
Conversation
|
Warning Rate limit exceeded@mkysel has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 53 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. 📒 Files selected for processing (1)
WalkthroughThis pull request involves multiple updates across various mock files and a test utility file. The primary changes include updating the code generation tool version from v2.44.1 to v2.50.0 in several mock files, modifying method signatures in the Changes
Possibly related PRs
Suggested reviewers
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
pkg/proto/mls_validation/v1/service.pb.gois excluded by!**/*.pb.go,!pkg/proto/**pkg/proto/mls_validation/v1/service_grpc.pb.gois excluded by!**/*.pb.go,!pkg/proto/**pkg/proto/openapi/mls_validation/v1/service.swagger.jsonis excluded by!pkg/proto/**pkg/proto/openapi/xmtpv4/payer_api/payer_api.swagger.jsonis excluded by!pkg/proto/**pkg/proto/xmtpv4/envelopes/envelopes.pb.gois excluded by!**/*.pb.go,!pkg/proto/**
📒 Files selected for processing (11)
.idea/vcs.xml(0 hunks)pkg/mocks/authn/mock_JWTVerifier.go(3 hunks)pkg/mocks/blockchain/mock_ChainClient.go(1 hunks)pkg/mocks/blockchain/mock_IBlockchainPublisher.go(1 hunks)pkg/mocks/indexer/mock_IBlockTracker.go(2 hunks)pkg/mocks/mls_validationv1/mock_ValidationApiClient.go(1 hunks)pkg/mocks/mlsvalidate/mock_MLSValidationService.go(1 hunks)pkg/mocks/registry/mock_NodeRegistry.go(5 hunks)pkg/mocks/registry/mock_NodesContract.go(1 hunks)pkg/mocks/storer/mock_LogStorer.go(1 hunks)pkg/testutils/envelopes/envelopes.go(2 hunks)
💤 Files with no reviewable changes (1)
- .idea/vcs.xml
✅ Files skipped from review due to trivial changes (7)
- pkg/mocks/storer/mock_LogStorer.go
- pkg/mocks/indexer/mock_IBlockTracker.go
- pkg/mocks/blockchain/mock_IBlockchainPublisher.go
- pkg/mocks/registry/mock_NodesContract.go
- pkg/mocks/blockchain/mock_ChainClient.go
- pkg/mocks/mlsvalidate/mock_MLSValidationService.go
- pkg/mocks/registry/mock_NodeRegistry.go
🔇 Additional comments (3)
pkg/mocks/authn/mock_JWTVerifier.go (1)
66-69: LGTM! Return type changes are properly implemented.The mock's Return and RunAndReturn methods correctly handle the new (uint32, error) signature.
Also applies to: 71-74
pkg/testutils/envelopes/envelopes.go (1)
33-33: Verify the consistency of DependsOn initialization.The DependsOn field is initialized differently in these functions:
- CreateClientEnvelope: new empty Cursor
- CreateOriginatorEnvelopeWithTopic: nil
Please verify if this difference is intentional based on the test requirements.
Also applies to: 133-133
✅ Verification successful
Different DependsOn initializations are intentionally testing both nil and non-nil cases
The proto definition confirms DependsOn as an optional field, making both nil and non-nil initializations valid. The different initializations in test utilities appear to be intentional to ensure test coverage for both scenarios.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other DependsOn field initializations rg 'DependsOn.*:' --type goLength of output: 339
pkg/mocks/mls_validationv1/mock_ValidationApiClient.go (1)
1-1: Verify impact of removed validation methods.The ValidateInboxIds and ValidateKeyPackages methods have been removed from the mock. Please ensure that no tests depend on these removed methods.
| @@ -1,4 +1,4 @@ | |||
| // Code generated by mockery v2.44.1. DO NOT EDIT. | |||
| // Code generated by mockery v2.50.0. DO NOT EDIT. | |||
There was a problem hiding this comment.
💡 Codebase verification
Test files need to be updated to handle the new Verify method signature
The production code correctly handles both return values, but test files need updates:
pkg/interceptors/server/auth_test.go: Mock expectations should return (uint32, error) instead of just errorpkg/authn/verifier_test.goandpkg/authn/claims_test.go: Tests should verify the returned nodeId value
🔗 Analysis chain
Verify that all callers handle the new return value from Verify method.
The JWT verification now returns an additional uint32 value. Ensure all calling code is updated to handle both return values correctly.
Also applies to: 28-45
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct calls to Verify method
rg -A 2 '\.Verify\(' --type go
Length of output: 3561
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/server/server_test.go (1)
Line range hint
110-110: Document the architectural implications of switching from LastSeen to DependsOn.The change from
LastSeentoDependsOnappears to be a significant architectural shift in how envelope dependencies are tracked. Consider:
- Adding documentation explaining the rationale behind this change
- Describing how this affects message ordering and dependencies
- Providing migration guidelines for existing implementations
Also applies to: 125-125, 164-164, 159-159, 173-173, 260-260
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pkg/api/message/publish_test.go(2 hunks)pkg/envelopes/envelopes_test.go(1 hunks)pkg/server/server_test.go(3 hunks)
🔇 Additional comments (2)
pkg/envelopes/envelopes_test.go (1)
110-110: Verify backward compatibility with existing clients.The change from
LastSeentoDependsOnin theAuthenticatedDatastructure might impact existing clients. Please ensure this change is backward compatible or that appropriate migration steps are documented.✅ Verification successful
Backward compatibility is properly maintained ✅
The codebase shows proper handling of the
LastSeenfield with null checks and default initializations in the service code. The change fromLastSeentoDependsOnappears to be test-specific and doesn't impact the actual service implementation.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining usages of LastSeen in the codebase echo "Checking for remaining LastSeen references in proto definitions..." rg -l "LastSeen" --type proto echo "Checking for LastSeen references in Go files..." rg -l "LastSeen" --type goLength of output: 576
Script:
#!/bin/bash # Check the context of LastSeen usage in the files echo "Checking LastSeen usage context in Go files..." rg "LastSeen" -B 2 -A 2 --type go echo -e "\nChecking for proto files with .proto extension..." fd -e proto echo -e "\nChecking content of proto files if found..." fd -e proto -x cat {}Length of output: 10336
pkg/api/message/publish_test.go (1)
125-125: LGTM! Test coverage looks good.The changes correctly update the test cases to use
DependsOninstead ofLastSeen, maintaining test coverage for both successful and failed key package validation scenarios.Also applies to: 164-164
Summary by CodeRabbit
Chores
Refactor
uint32value.LastSeenwithDependsOn.