Tags for services #93
Conversation
|
Note Reviews pausedUse the following commands to manage reviews:
📝 WalkthroughWalkthroughThis pull request updates multiple components for gRPC v3 compatibility and clearer message routing. The partner-plugin server now uses v3 service definitions, with corresponding import path and signature modifications. Dependency versions in the module file have been updated. Messaging components are enhanced by renaming methods to distinguish P2P versus local messages and by adding new request-processing functions. Additionally, the RPC interface and server now expose separate methods for handling P2P and local messages. The gRPC code generation scripts and templates have been restructured with new templates and constant definitions to support these changes. Changes
Suggested reviewers
Poem
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 (
|
|
Hi Vjera and Alex,
I looked at the code snippets below and first thing that struck me is that RetailerCode and SubSupplierCode can be left empty in our examples (not include them) as in most cases they'll not be used. Do not use unspecified code types in our examples.
If SwissAir offers a combination of a trainticket and a flight to travel from a small village to JFK airport in New York the would have for example:
St. Gallen station: https://www.sbb.ch/en/travel-information/stations/find-station/st-gallen-station.html
we would take for example the B 151 at 10h28 on 15 March to ZRH airport
(note that you can book the B 151 also for 10h58 and 11h28,...)
That would lead to this example:
Segments: []*transportv3.Segment{
{ SegmentId: "SEG789",
ProviderCode: "SBB",
ProductCode: &typesv2.ProductCode
{Code: "B",
Number: 789,
Type: typesv2.ProductCodeType_PRODUCT_CODE_TYPE_SUPPLIER,
But I am happy to do a session and walk through the fields and values used....
Kind regards,
Sam
[A black and white logo Description automatically generated]
Sam Jaarsma
VP Standards
T
:
+34 660 594305
M
:
Book a meeting with ***@***.***?anonymous&ep=plink>
W
:
https://www.chain4travel.com<https://www.chain4travel.com/>
https://caminoscan.com/
Chain4Travel AG
Dammstrasse 16 I CH-6300 Zug
CHE-364.921.803
Represented by: Pablo Castillo
This e-mail may contain confidential and/or privileged information. If you are not the intended recipient (or have received this e-mail in error) please notify the sender immediately and destroy this e-mail. Any unauthorised copying, disclosure or distribution of the material in this e-mail is strictly forbidden.
________________________________
From: Vjera Turk ***@***.***>
Sent: Wednesday, February 5, 2025 17:06
To: chain4travel/camino-messenger-bot ***@***.***>
Cc: Sam Jaarsma ***@***.***>; Mention ***@***.***>
Subject: Re: [chain4travel/camino-messenger-bot] Tags for services (PR #93)
@VjeraTurk commented on this pull request.
________________________________
In examples/rpc/partner-plugin/server.go<#93 (comment)>:
+ },
+ }, {
+ SupplierCode: &typesv2.SupplierProductCode{
+ SupplierCode: "XY",
+ SupplierNumber: 789,
+ },
+ Segments: []*transportv3.Segment{
+ {
+ SegmentId: "SEG789",
+ ProviderCode: "DB",
+ RetailerCode: "NS",
+ SubSupplierCode: "SNCF",
+ ProductCode: &typesv2.ProductCode{
+ Code: "XY",
+ Number: 789,
+ Type: typesv2.ProductCodeType_PRODUCT_CODE_TYPE_UNSPECIFIED,
@Noctunus<https://github.com/Noctunus> The pp-mock serves as a new plugin example and it focuses on showing handlers and filtering logic but not on the full form of the proto-response message.
In the old examples server.go I just try to fill all the fields of the proto-response message to demonstrate nothing more than the use of types and the full form of the response, with values somewhat true to what an actual response might look like.
I try to avoid making examples that would imply British Airways BA/BWA is a railway operator or accommodation provider, that's mainly why I opt for unspecified - it being a valid option.
// IATA airport code
LOCATION_CODE_TYPE_IATA_CODE = 2;
LOCATION_CODE_TYPE_3ALPHA_CODE = 4;
// A 6-digit code for train stations generally used for retail purposes
After finishing mocks, we could have @SamJaarsma<https://github.com/SamJaarsma> give his input on the values added to the examples before a technical review? What do you think?
—
Reply to this email directly, view it on GitHub<#93 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/A5HVVV4SADKA4GKTAWAIX732OIZJ7AVCNFSM6AAAAABWA32IVSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDKOJWGI4DENBSHA>.
You are receiving this because you were mentioned.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (3)
pp-mock/handlers/accommodation/v1/accommodation_search.go (1)
231-232: Fix potential panic in product code filtering.The code assumes that
prop.Property.ProductCodesis non-empty. This could cause a panic if a property has no product codes.Apply this diff to add bounds checking:
- if len(prop.Property.ProductCodes) > 0 && prop.Property.ProductCodes[0].Code == code.Code { + if prop.Property.ProductCodes[0].Code == code.Code {pp-mock/handlers/accommodation/v2/accommodation_search.go (2)
232-232: Fix potential panic in product code filtering.Similar to v1, the code assumes non-empty ProductCodes array, which could cause a panic.
Apply this diff to add bounds checking:
- if prop.Property.ProductCodes[0].Code == code.Code { + if len(prop.Property.ProductCodes) > 0 && prop.Property.ProductCodes[0].Code == code.Code {
205-205: Add nil check for GeoTree in address.The code assumes address.GeoTree is not nil, which could cause a panic.
Apply this diff to add nil check:
- if address.GeoTree.CityOrResort == geoTreeLocation.CityOrResort && address.GeoTree.Country == geoTreeLocation.Country && address.GeoTree.Region == geoTreeLocation.Region { + if address.GeoTree != nil && address.GeoTree.CityOrResort == geoTreeLocation.CityOrResort && address.GeoTree.Country == geoTreeLocation.Country && address.GeoTree.Region == geoTreeLocation.Region {
♻️ Duplicate comments (1)
internal/messaging/processor.go (1)
295-297: Address the TODO comment about error handling.This TODO comment was previously flagged by Noctunus. Consider moving the error header addition earlier in the process for better error handling.
Apply this diff to improve error handling:
- // TODO: @VjeraTurk or AddErrorToResponseHeader earlier?! - p.responseHandler.AddErrorToResponseHeader(responseMsg.Content, err.Error()) - return ctx, responseMsg, fmt.Errorf("failed to process request message: %w", err) + errMsg := fmt.Errorf("failed to process request message: %w", err) + p.responseHandler.AddErrorToResponseHeader(responseMsg.Content, errMsg.Error()) + return ctx, responseMsg, errMsg
🧹 Nitpick comments (9)
pp-mock/handlers/accommodation/v1/accommodation_search.go (1)
87-99: Optimize duplicate checking performance.The nested loop for checking duplicate supplier codes has O(n²) complexity. Using a map would reduce this to O(n).
Consider this optimization:
+ supplierCodeMap := make(map[string]bool) for _, prop := range filteredProps { - exists := false - for _, existingProp := range availableProperties { - if existingProp.Property.SupplierCode.SupplierCode == prop.Property.SupplierCode.SupplierCode { - exists = true - break - } - } - if !exists { + if !supplierCodeMap[prop.Property.SupplierCode.SupplierCode] { + supplierCodeMap[prop.Property.SupplierCode.SupplierCode] = true availableProperties = append(availableProperties, prop) } }examples/rpc/partner-plugin/server.go (5)
103-225: Detailed stub response inActivityProductInfo.Lines 103 onward add a rich stub response with multiple nested fields (Activities, Zones, Descriptions, etc.). It demonstrates the structure of a potential real-world response. From a maintainability perspective, consider whether a smaller, more focused example or JSON serialization approach could be clearer.
387-426:AccommodationSearchextended results.This function returns a more robust search result, including
cancelPolicy,travellers, etc. While thorough as an example, consider adding minimal comments or docstrings clarifying the placeholders for developers who might adapt this snippet.
514-767: RichTransportSearchResponsestructure (first large block).This block enumerates multiple segments, baggage details, taxes, observational remarks, and deeper service facts. The example is extensive but can be helpful in illustrating usage. Watch out for potential confusion around repeated codes like
RetailerCode,SubSupplierCodebeing set to the same values asProviderCode; Sam’s remarks suggest these are often empty in real usage.
930-1048:SeatMapwith seat layout detail.You introduce a thorough seat mapping example with sections, seat features, localized attributes, and images. The example is quite large, but it effectively demonstrates advanced seat mapping. Checking that overly detailed stubs don’t confuse plugin developers is recommended.
1106-1133:SeatMapAvailabilitystub.Lines 1115–1133 introduce seat map inventory placeholders. Overall consistent with the seat map approach. Possibly consider referencing real constraints or a back-end system if these seats truly reflect actual usage.
pp-mock/handlers/accommodation/v1/accommodation_product_list.go (1)
41-47: LGTM! Good error handling improvement.The changes simplify the code by removing unnecessary filtering while adding proper error handling for empty properties. Consider adding a debug log before returning the error to help with troubleshooting.
if len(properties) == 0 { + h.logger.Debug("No properties found in mockdata.PropertiesV1") return nil, status.Errorf(codes.NotFound, "no properties found") }Also applies to: 53-53
internal/messaging/response_handler.go (1)
127-132: Implement the placeholder method.The
ProcessRequestMessagemethod is currently a placeholder with a TODO comment. Consider implementing the logic for processing request messages, particularly for cancellation messages as mentioned in the comment.Would you like me to help implement the logic for processing request messages, starting with cancellation messages?
internal/messaging/processor.go (1)
293-294: Remove redundant logging statements.The logging statements before and after processing are redundant and may clutter the logs.
Apply this diff to clean up the logging:
- p.logger.Infof("responseMsg before processing %v", responseMsg.Content) if err := p.responseHandler.ProcessRequestMessage(ctx, responseMsg, requestMsg.Content); err != nil { - p.logger.Infof("responseMsg after processing %v", responseMsg.Content)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (136)
go.sumis excluded by!**/*.suminternal/rpc/generated/accommodationv1_AccommodationProductInfoService_AccommodationProductInfo_server_method.gois excluded by!**/generated/**internal/rpc/generated/accommodationv1_AccommodationProductInfoService_client.gois excluded by!**/generated/**internal/rpc/generated/accommodationv1_AccommodationProductInfoService_server.gois excluded by!**/generated/**internal/rpc/generated/accommodationv1_AccommodationProductListService_AccommodationProductList_server_method.gois excluded by!**/generated/**internal/rpc/generated/accommodationv1_AccommodationProductListService_client.gois excluded by!**/generated/**internal/rpc/generated/accommodationv1_AccommodationProductListService_server.gois excluded by!**/generated/**internal/rpc/generated/accommodationv1_AccommodationSearchService_AccommodationSearch_server_method.gois excluded by!**/generated/**internal/rpc/generated/accommodationv1_AccommodationSearchService_client.gois excluded by!**/generated/**internal/rpc/generated/accommodationv1_AccommodationSearchService_server.gois excluded by!**/generated/**internal/rpc/generated/accommodationv2_AccommodationProductInfoService_AccommodationProductInfo_server_method.gois excluded by!**/generated/**internal/rpc/generated/accommodationv2_AccommodationProductInfoService_client.gois excluded by!**/generated/**internal/rpc/generated/accommodationv2_AccommodationProductInfoService_server.gois excluded by!**/generated/**internal/rpc/generated/accommodationv2_AccommodationProductListService_AccommodationProductList_server_method.gois excluded by!**/generated/**internal/rpc/generated/accommodationv2_AccommodationProductListService_client.gois excluded by!**/generated/**internal/rpc/generated/accommodationv2_AccommodationProductListService_server.gois excluded by!**/generated/**internal/rpc/generated/accommodationv2_AccommodationSearchService_AccommodationSearch_server_method.gois excluded by!**/generated/**internal/rpc/generated/accommodationv2_AccommodationSearchService_client.gois excluded by!**/generated/**internal/rpc/generated/accommodationv2_AccommodationSearchService_server.gois excluded by!**/generated/**internal/rpc/generated/accommodationv3_AccommodationProductInfoService_AccommodationProductInfo_client_method.gois excluded by!**/generated/**internal/rpc/generated/accommodationv3_AccommodationProductInfoService_AccommodationProductInfo_server_method.gois excluded by!**/generated/**internal/rpc/generated/accommodationv3_AccommodationProductInfoService_client.gois excluded by!**/generated/**internal/rpc/generated/accommodationv3_AccommodationProductInfoService_server.gois excluded by!**/generated/**internal/rpc/generated/accommodationv3_AccommodationProductListService_AccommodationProductList_client_method.gois excluded by!**/generated/**internal/rpc/generated/accommodationv3_AccommodationProductListService_AccommodationProductList_server_method.gois excluded by!**/generated/**internal/rpc/generated/accommodationv3_AccommodationProductListService_client.gois excluded by!**/generated/**internal/rpc/generated/accommodationv3_AccommodationProductListService_server.gois excluded by!**/generated/**internal/rpc/generated/accommodationv3_AccommodationSearchService_AccommodationSearch_client_method.gois excluded by!**/generated/**internal/rpc/generated/accommodationv3_AccommodationSearchService_AccommodationSearch_server_method.gois excluded by!**/generated/**internal/rpc/generated/accommodationv3_AccommodationSearchService_client.gois excluded by!**/generated/**internal/rpc/generated/accommodationv3_AccommodationSearchService_server.gois excluded by!**/generated/**internal/rpc/generated/activityv1_ActivityProductInfoService_ActivityProductInfo_server_method.gois excluded by!**/generated/**internal/rpc/generated/activityv1_ActivityProductInfoService_client.gois excluded by!**/generated/**internal/rpc/generated/activityv1_ActivityProductInfoService_server.gois excluded by!**/generated/**internal/rpc/generated/activityv1_ActivityProductListService_ActivityProductList_server_method.gois excluded by!**/generated/**internal/rpc/generated/activityv1_ActivityProductListService_client.gois excluded by!**/generated/**internal/rpc/generated/activityv1_ActivityProductListService_server.gois excluded by!**/generated/**internal/rpc/generated/activityv1_ActivitySearchService_ActivitySearch_server_method.gois excluded by!**/generated/**internal/rpc/generated/activityv1_ActivitySearchService_client.gois excluded by!**/generated/**internal/rpc/generated/activityv1_ActivitySearchService_server.gois excluded by!**/generated/**internal/rpc/generated/activityv2_ActivityProductInfoService_ActivityProductInfo_server_method.gois excluded by!**/generated/**internal/rpc/generated/activityv2_ActivityProductInfoService_client.gois excluded by!**/generated/**internal/rpc/generated/activityv2_ActivityProductInfoService_server.gois excluded by!**/generated/**internal/rpc/generated/activityv2_ActivityProductListService_ActivityProductList_server_method.gois excluded by!**/generated/**internal/rpc/generated/activityv2_ActivityProductListService_client.gois excluded by!**/generated/**internal/rpc/generated/activityv2_ActivityProductListService_server.gois excluded by!**/generated/**internal/rpc/generated/activityv2_ActivitySearchService_ActivitySearch_server_method.gois excluded by!**/generated/**internal/rpc/generated/activityv2_ActivitySearchService_client.gois excluded by!**/generated/**internal/rpc/generated/activityv2_ActivitySearchService_server.gois excluded by!**/generated/**internal/rpc/generated/activityv3_ActivityProductInfoService_ActivityProductInfo_client_method.gois excluded by!**/generated/**internal/rpc/generated/activityv3_ActivityProductInfoService_ActivityProductInfo_server_method.gois excluded by!**/generated/**internal/rpc/generated/activityv3_ActivityProductInfoService_client.gois excluded by!**/generated/**internal/rpc/generated/activityv3_ActivityProductInfoService_server.gois excluded by!**/generated/**internal/rpc/generated/activityv3_ActivityProductListService_ActivityProductList_client_method.gois excluded by!**/generated/**internal/rpc/generated/activityv3_ActivityProductListService_ActivityProductList_server_method.gois excluded by!**/generated/**internal/rpc/generated/activityv3_ActivityProductListService_client.gois excluded by!**/generated/**internal/rpc/generated/activityv3_ActivityProductListService_server.gois excluded by!**/generated/**internal/rpc/generated/activityv3_ActivitySearchService_ActivitySearch_client_method.gois excluded by!**/generated/**internal/rpc/generated/activityv3_ActivitySearchService_ActivitySearch_server_method.gois excluded by!**/generated/**internal/rpc/generated/activityv3_ActivitySearchService_client.gois excluded by!**/generated/**internal/rpc/generated/activityv3_ActivitySearchService_server.gois excluded by!**/generated/**internal/rpc/generated/bookv1_MintService_Mint_server_method.gois excluded by!**/generated/**internal/rpc/generated/bookv1_MintService_client.gois excluded by!**/generated/**internal/rpc/generated/bookv1_MintService_server.gois excluded by!**/generated/**internal/rpc/generated/bookv1_ValidationService_Validation_server_method.gois excluded by!**/generated/**internal/rpc/generated/bookv1_ValidationService_client.gois excluded by!**/generated/**internal/rpc/generated/bookv1_ValidationService_server.gois excluded by!**/generated/**internal/rpc/generated/bookv2_MintService_Mint_server_method.gois excluded by!**/generated/**internal/rpc/generated/bookv2_MintService_client.gois excluded by!**/generated/**internal/rpc/generated/bookv2_MintService_server.gois excluded by!**/generated/**internal/rpc/generated/bookv2_ValidationService_Validation_server_method.gois excluded by!**/generated/**internal/rpc/generated/bookv2_ValidationService_client.gois excluded by!**/generated/**internal/rpc/generated/bookv2_ValidationService_server.gois excluded by!**/generated/**internal/rpc/generated/bookv3_MintService_Mint_client_method.gois excluded by!**/generated/**internal/rpc/generated/bookv3_MintService_Mint_server_method.gois excluded by!**/generated/**internal/rpc/generated/bookv3_MintService_client.gois excluded by!**/generated/**internal/rpc/generated/bookv3_MintService_server.gois excluded by!**/generated/**internal/rpc/generated/bookv3_ValidationService_Validation_client_method.gois excluded by!**/generated/**internal/rpc/generated/bookv3_ValidationService_Validation_server_method.gois excluded by!**/generated/**internal/rpc/generated/bookv3_ValidationService_client.gois excluded by!**/generated/**internal/rpc/generated/bookv3_ValidationService_server.gois excluded by!**/generated/**internal/rpc/generated/infov1_CountryEntryRequirementsService_CountryEntryRequirements_server_method.gois excluded by!**/generated/**internal/rpc/generated/infov1_CountryEntryRequirementsService_client.gois excluded by!**/generated/**internal/rpc/generated/infov1_CountryEntryRequirementsService_server.gois excluded by!**/generated/**internal/rpc/generated/infov2_CountryEntryRequirementsService_CountryEntryRequirements_server_method.gois excluded by!**/generated/**internal/rpc/generated/infov2_CountryEntryRequirementsService_client.gois excluded by!**/generated/**internal/rpc/generated/infov2_CountryEntryRequirementsService_server.gois excluded by!**/generated/**internal/rpc/generated/insurancev1_InsuranceProductInfoService_InsuranceProductInfo_server_method.gois excluded by!**/generated/**internal/rpc/generated/insurancev1_InsuranceProductInfoService_client.gois excluded by!**/generated/**internal/rpc/generated/insurancev1_InsuranceProductInfoService_server.gois excluded by!**/generated/**internal/rpc/generated/insurancev1_InsuranceProductListService_InsuranceProductList_server_method.gois excluded by!**/generated/**internal/rpc/generated/insurancev1_InsuranceProductListService_client.gois excluded by!**/generated/**internal/rpc/generated/insurancev1_InsuranceProductListService_server.gois excluded by!**/generated/**internal/rpc/generated/insurancev1_InsuranceSearchService_InsuranceSearch_server_method.gois excluded by!**/generated/**internal/rpc/generated/insurancev1_InsuranceSearchService_client.gois excluded by!**/generated/**internal/rpc/generated/insurancev1_InsuranceSearchService_server.gois excluded by!**/generated/**internal/rpc/generated/pingv1_PingService_Ping_server_method.gois excluded by!**/generated/**internal/rpc/generated/pingv1_PingService_client.gois excluded by!**/generated/**internal/rpc/generated/pingv1_PingService_server.gois excluded by!**/generated/**internal/rpc/generated/register_client_services.gois excluded by!**/generated/**internal/rpc/generated/register_server_services.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv1_SeatMapAvailabilityService_SeatMapAvailability_server_method.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv1_SeatMapAvailabilityService_client.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv1_SeatMapAvailabilityService_server.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv1_SeatMapService_SeatMap_server_method.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv1_SeatMapService_client.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv1_SeatMapService_server.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv2_SeatMapAvailabilityService_SeatMapAvailability_server_method.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv2_SeatMapAvailabilityService_client.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv2_SeatMapAvailabilityService_server.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv2_SeatMapService_SeatMap_server_method.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv2_SeatMapService_client.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv2_SeatMapService_server.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv3_SeatMapAvailabilityService_SeatMapAvailability_client_method.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv3_SeatMapAvailabilityService_SeatMapAvailability_server_method.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv3_SeatMapAvailabilityService_client.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv3_SeatMapAvailabilityService_server.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv3_SeatMapService_SeatMap_client_method.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv3_SeatMapService_SeatMap_server_method.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv3_SeatMapService_client.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv3_SeatMapService_server.gois excluded by!**/generated/**internal/rpc/generated/transportv1_TransportSearchService_TransportSearch_server_method.gois excluded by!**/generated/**internal/rpc/generated/transportv1_TransportSearchService_client.gois excluded by!**/generated/**internal/rpc/generated/transportv1_TransportSearchService_server.gois excluded by!**/generated/**internal/rpc/generated/transportv2_TransportSearchService_TransportSearch_server_method.gois excluded by!**/generated/**internal/rpc/generated/transportv2_TransportSearchService_client.gois excluded by!**/generated/**internal/rpc/generated/transportv2_TransportSearchService_server.gois excluded by!**/generated/**internal/rpc/generated/transportv3_TransportProductListService_TransportProductList_client_method.gois excluded by!**/generated/**internal/rpc/generated/transportv3_TransportProductListService_TransportProductList_server_method.gois excluded by!**/generated/**internal/rpc/generated/transportv3_TransportProductListService_client.gois excluded by!**/generated/**internal/rpc/generated/transportv3_TransportProductListService_server.gois excluded by!**/generated/**internal/rpc/generated/transportv3_TransportSearchService_TransportSearch_client_method.gois excluded by!**/generated/**internal/rpc/generated/transportv3_TransportSearchService_TransportSearch_server_method.gois excluded by!**/generated/**internal/rpc/generated/transportv3_TransportSearchService_client.gois excluded by!**/generated/**internal/rpc/generated/transportv3_TransportSearchService_server.gois excluded by!**/generated/**internal/rpc/generated/unmarshal.gois excluded by!**/generated/**
📒 Files selected for processing (21)
examples/rpc/partner-plugin/server.go(33 hunks)go.mod(2 hunks)internal/app/app.go(1 hunks)internal/messaging/mint_v1.go(1 hunks)internal/messaging/mint_v2.go(1 hunks)internal/messaging/noop_response_handler.go(1 hunks)internal/messaging/processor.go(4 hunks)internal/messaging/processor_test.go(2 hunks)internal/messaging/response_handler.go(2 hunks)internal/rpc/rpc.go(1 hunks)internal/rpc/server/server.go(5 hunks)pp-mock/handlers/accommodation/v1/accommodation_product_list.go(1 hunks)pp-mock/handlers/accommodation/v1/accommodation_search.go(1 hunks)pp-mock/handlers/accommodation/v2/accommodation_product_list.go(1 hunks)pp-mock/handlers/accommodation/v2/accommodation_search.go(1 hunks)scripts/generate_grpc_service_handlers.sh(10 hunks)templates/client.go.tpl(0 hunks)templates/client_method.go.tpl(1 hunks)templates/server.go.tpl(1 hunks)templates/server_local_on_chain_method.go.tpl(1 hunks)templates/server_p2p_method.go.tpl(1 hunks)
💤 Files with no reviewable changes (1)
- templates/client.go.tpl
🧰 Additional context used
🧠 Learnings (1)
examples/rpc/partner-plugin/server.go (1)
Learnt from: VjeraTurk
PR: chain4travel/camino-messenger-bot#89
File: pp-mock/handlers/accommodation/v3/accommodation_search.go:148-160
Timestamp: 2025-01-22T09:22:31.686Z
Learning: The AccommodationSearch service in v3 intentionally reuses the AccommodationSearchResult type from v2 as per the protocol definition. This is not a version compatibility issue.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Unit Tests
- GitHub Check: Static Analysis
🔇 Additional comments (36)
pp-mock/handlers/accommodation/v1/accommodation_search.go (1)
77-78: LGTM! Good fix for query-specific property isolation.Moving the
availablePropertiesdeclaration inside the query loop ensures that each query starts with a fresh set of properties, preventing unintended property carryover between queries.pp-mock/handlers/accommodation/v2/accommodation_search.go (1)
78-78: LGTM! Consistent with v1 implementation.The change maintains consistency with the v1 handler by ensuring query-specific property isolation.
examples/rpc/partner-plugin/server.go (13)
15-16: Updated imports to v3 packages.All these lines reference the new v3 service packages for accommodation, activity, seat map, transport, etc. This ensures compatibility with the latest proto definitions. Make sure to confirm that all related message usages are updated to v3 structures throughout the codebase.
Also applies to: 25-26, 28-29, 39-40, 42-43
52-52: Struct extension for new v3 services.The
partnerPluginstruct now declares multiple new fields for v3 services (activity, accommodation, seat_map, transport). This expansion appears consistent with the updated import references.
58-61: New gRPC service server interfaces.By adding these v3 server interfaces (e.g.,
ActivitySearchServiceServer,AccommodationSearchServiceServer,TransportProductListServiceServer, etc.), the plugin is now aligned with the upgraded service definitions. Be sure that each server method is implemented or stubbed appropriately to avoid runtime panics.Also applies to: 64-65, 67-67, 69-69
281-281:ActivityProductListstub response.These lines introduce a placeholder list of activities. The logic is straightforward and returns a single activity for illustration. This looks consistent with the approach taken in
ActivityProductInfo. No immediate issues.Also applies to: 290-294
320-320:ActivitySearchwith empty results.Here, the response includes a simple success header and
Metadatawith aSearchId. The code is minimal but consistent with a search pattern. No issues spotted.Also applies to: 329-334
341-341:AccommodationProductInforesponding with PaymentType.This stub provides a minimal property info array with a
PaymentType. The code is correct syntactically.Also applies to: 350-352
359-359:AccommodationProductListexample.Returns a static property named “Hotel”. This is consistent with the higher-level stubbing pattern. Looks fine.
Also applies to: 368-369
494-501: TransportSearch v3 logging.Line 501 logs “(TransportSearch) v3,” clarifying the version used, which is helpful for debugging. Ensure that earlier references to
transportv2grpc.TransportSearchServiceServerare fully replaced if you no longer support v2.
503-513: Basic header and metadata generation.These lines set up a standard success header and attach search metadata. No concerns with the approach; it aligns with prior code.
768-776: Traveller listing in the same response.Providing sample travellers helps demonstrate the structure. Nothing problematic found; the approach is consistent with the rest of the stub.
784-925:TransportProductListResponsebuilding out multiple trip segments.This uses v3 references, demonstrating how multiple segments can be returned. Again, watch for optional fields like
RetailerCodein real usage. The code is functionally correct.
1154-1156: Response header inCountryEntryRequirements.These lines specify a success status. This pattern is uniform across other calls. No concerns.
1316-1326: Registration of new v3 service servers.The addition of
RegisterActivityProductInfoServiceServer,RegisterTransportSearchServiceServer, etc., ensures that v3 RPC endpoints become available. This matches the new methods you implemented.templates/server_p2p_method.go.tpl (1)
14-14: Switch toHandleP2PMessageRequest.Line 14 changes the handler invocation from
HandleRequesttoHandleP2PMessageRequest, clarifying that this method now specializes in peer-to-peer requests. This is consistent with the PR’s objective of distinguishing p2p services.templates/server_local_on_chain_method.go.tpl (1)
1-25: New local on-chain method template.This entire file is new, creating a server method that calls
HandleMessageRequest. The pattern mirrors the P2P template but focuses on “local on-chain” logic. The error handling and type checks look solid. Overall, an appropriate extension of the generation framework.internal/messaging/noop_response_handler.go (2)
21-21: LGTM!The implementation correctly returns
nilfor this no-op handler.
23-25: LGTM!The implementation correctly returns
nilfor this no-op handler, maintaining consistency with other no-op methods in the file.templates/server.go.tpl (2)
9-9: LGTM!The import is correctly added and required for the new message type constants.
14-18: LGTM!The constants are well-structured and follow the naming convention. The message types are correctly derived from the service version using template variables.
templates/client_method.go.tpl (1)
35-35: LGTM!The newline addition follows standard file formatting practices.
internal/rpc/rpc.go (1)
18-19: LGTM! Clear separation of concerns.The split into
HandleP2PMessageRequestandHandleMessageRequestmethods improves clarity by explicitly distinguishing between P2P and local message handling. This aligns well with the PR objectives to enhance service handlers with better categorization.pp-mock/handlers/accommodation/v2/accommodation_product_list.go (1)
41-47: Consider extracting common logic to reduce duplication.The property handling logic is identical between v1 and v2 implementations.
Consider extracting the common logic into a shared helper function:
+func handleProperties[T any](properties []T) ([]*T, error) { + result := make([]*T, 0, len(properties)) + for _, property := range properties { + result = append(result, property.Property) + } + if len(result) == 0 { + return nil, status.Errorf(codes.NotFound, "no properties found") + } + return result, nil +}Also applies to: 53-53
internal/messaging/mint_v1.go (1)
28-33: LGTM! Good input validation.The addition of BuyerAddress validation prevents potential issues with invalid addresses early in the process.
internal/messaging/mint_v2.go (1)
30-36: Extract common validation logic to reduce duplication.As noted in a previous review by Noctunus, there's code duplication between v1 and v2. The BuyerAddress validation logic is identical in both versions.
Consider extracting the validation into a shared helper function:
+func validateBuyerAddress(address string) error { + if !common.IsHexAddress(address) { + return fmt.Errorf("Invalid BuyerAddress: %s", address) + } + return nil +}Then use it in both v1 and v2:
-if !common.IsHexAddress(request.BuyerAddress) { - errMsg := fmt.Sprintf("Invalid BuyerAddress: %s", request.BuyerAddress) - h.logger.Error(errMsg) - h.AddErrorToResponseHeader(response, errMsg) - return -} +if err := validateBuyerAddress(request.BuyerAddress); err != nil { + h.logger.Error(err) + h.AddErrorToResponseHeader(response, err.Error()) + return +}internal/rpc/server/server.go (1)
73-76: LGTM! Good security practice with conditional reflection service.The conditional registration of the gRPC reflection service based on developer mode is a good security practice, as it ensures the service is only available in development environments.
internal/app/app.go (1)
173-173: LGTM! Clean integration of developer mode configuration.The addition of the developer mode parameter to the RPC server initialization is clean and consistent with the server implementation.
internal/messaging/processor_test.go (1)
54-54: LGTM! The test function and method call have been correctly renamed.The renaming from
TestProcessIncomingMessagetoTestProcessIncomingP2PMessageand the corresponding method call update align with the PR's objective of distinguishing between p2p and local services.Also applies to: 211-211
go.mod (1)
6-7: Verify the validity and security of the updated dependency versions.The following dependency versions have been updated:
buf.build/gen/go/chain4travel/camino-messenger-protocol/grpc/go→v1.5.1-00000000000000-a57016294956.1buf.build/gen/go/chain4travel/camino-messenger-protocol/protocolbuffers/go→v1.36.4-00000000000000-66f585c173ec.1google.golang.org/protobuf→v1.36.4Let's check for any security advisories and ensure these are the latest stable versions:
Also applies to: 103-103
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check for security advisories and latest versions of protobuf # Check for security advisories gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: GO, package: "google.golang.org/protobuf") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }' # Check latest version from pkg.go.dev curl -s "https://pkg.go.dev/google.golang.org/protobuf?tab=versions" | grep -o 'v[0-9]\+\.[0-9]\+\.[0-9]\+'Length of output: 1487
Dependency Versions Verified as Secure
- The
google.golang.org/protobufupdate to v1.36.4 is above the vulnerable ranges (fixed in v1.29.1 and v1.33.0). Although pkg.go.dev shows v1.36.5 as the latest, v1.36.4 remains secure.- The pseudo-versions for the buf.build dependencies (
buf.build/gen/go/chain4travel/camino-messenger-protocol/grpc/goand.../protocolbuffers/go) appear generated from known commits and show no known security advisories.scripts/generate_grpc_service_handlers.sh (6)
8-9: LGTM! New templates added for P2P and local on-chain services.The addition of separate templates for different service types improves modularity and maintainability.
11-13: Address the TODO comment about path separation.Based on the past review comments, it was discussed that keeping P2P and local services in the same path is acceptable for now. However, this should be revisited if the need for separation arises in the future.
19-20: Address the TODO comment about claim imports.Based on the past review comments, this issue is still unresolved and requires attention. Additionally, the comment about adding
| tail -n 1needs to be addressed.
315-327: LGTM! Service tag extraction implementation.The implementation correctly extracts service type, routing, and on-chain status from the custom annotation line. This aligns with the PR objectives of enhancing service categorization.
54-141: LGTM! Comprehensive routing-based code generation.The implementation correctly handles different routing types:
- P2P services: Generates both client and server code
- Local on-chain services: Generates only server code
- Handles empty responses by importing the
emptypbpackage when needed
378-381: LGTM! Conditional service registration.The implementation correctly:
- Registers unmarshalling methods only for P2P services
- Adds services to registration only for P2P routing
This aligns with the PR objectives of distinguishing between P2P and local services.Also applies to: 398-401
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/rpc/server/server.go (2)
108-127: Consider refactoring duplicated message handling logic.Both message handling methods share significant code duplication in their structure, error handling, and metadata processing. Consider extracting the common logic into a helper method.
Here's a suggested refactoring:
+func (s *server) handleMessage( + ctx context.Context, + spanName string, + requestType types.MessageType, + request protoreflect.ProtoMessage, + processor func(context.Context, *types.Message) (*types.Message, error), +) (protoreflect.ProtoMessage, error) { + ctx, span := s.tracer.Start(ctx, spanName, trace.WithSpanKind(trace.SpanKindServer)) + defer span.End() + md, err := s.processMetadata(ctx, s.tracer.TraceIDForSpan(span)) + if err != nil { + return nil, fmt.Errorf("error processing metadata: %w", err) + } + + response, err := processor(ctx, &types.Message{ + Type: requestType, + Content: request, + Metadata: md, + }) + if err != nil { + return nil, fmt.Errorf("error processing request: %w", err) + } + response.Metadata.Stamp(fmt.Sprintf("%s-%s", s.Checkpoint(), "processed")) + err = grpc.SendHeader(ctx, response.Metadata.ToGrpcMD()) + return response.Content, err +} + func (s *server) HandleP2PMessageRequest(ctx context.Context, requestType types.MessageType, request protoreflect.ProtoMessage) (protoreflect.ProtoMessage, error) { - ctx, span := s.tracer.Start(ctx, "server.HandleP2PMessageRequest", trace.WithSpanKind(trace.SpanKindServer)) - defer span.End() - md, err := s.processMetadata(ctx, s.tracer.TraceIDForSpan(span)) - if err != nil { - return nil, fmt.Errorf("error processing metadata: %w", err) - } - - response, err := s.processor.SendRequestMessage(ctx, &types.Message{ - Type: requestType, - Content: request, - Metadata: md, - }) - if err != nil { - return nil, fmt.Errorf("error processing outbound request: %w", err) - } - response.Metadata.Stamp(fmt.Sprintf("%s-%s", s.Checkpoint(), "processed")) - err = grpc.SendHeader(ctx, response.Metadata.ToGrpcMD()) - return response.Content, err + return s.handleMessage(ctx, "server.HandleP2PMessageRequest", requestType, request, s.processor.SendRequestMessage) } func (s *server) HandleMessageRequest(ctx context.Context, requestType types.MessageType, request protoreflect.ProtoMessage) (protoreflect.ProtoMessage, error) { - ctx, span := s.tracer.Start(ctx, "server.HandleMessageRequest", trace.WithSpanKind(trace.SpanKindServer)) - defer span.End() - md, err := s.processMetadata(ctx, s.tracer.TraceIDForSpan(span)) - if err != nil { - return nil, fmt.Errorf("error processing metadata: %w", err) - } - response, err := s.processor.ProcessIncomingLocalMessage(ctx, &types.Message{ - Type: requestType, - Content: request, - Metadata: md, - }) - if err != nil { - return nil, fmt.Errorf("error processing request: %w", err) - } - response.Metadata.Stamp(fmt.Sprintf("%s-%s", s.Checkpoint(), "processed")) - err = grpc.SendHeader(ctx, response.Metadata.ToGrpcMD()) - return response.Content, err + return s.handleMessage(ctx, "server.HandleMessageRequest", requestType, request, s.processor.ProcessIncomingLocalMessage) }Also applies to: 129-147
126-126: Address the TODO comment about gRPC error codes.The TODO comments indicate a need to map errors to specific gRPC status codes. This is important for proper error handling on the client side.
Would you like me to help implement proper gRPC error code mapping? Here's what we can do:
- Map domain-specific errors to appropriate gRPC status codes
- Implement error wrapping with
status.Error- Add error code documentation
Also applies to: 146-146
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/rpc/server/server.go(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Static Analysis
- GitHub Check: Build docker image
- GitHub Check: Unit Tests
🔇 Additional comments (1)
internal/rpc/server/server.go (1)
24-24: LGTM! Good security practice with conditional reflection service.The conditional registration of the gRPC reflection service based on
developerModeis a good security practice, as it ensures the service is only available during development.Please ensure that
developerModeis set tofalsein production environments to prevent potential security risks.Also applies to: 47-47, 73-76
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
scripts/generate_grpc_service_handlers.sh (1)
344-345: COMMON_TYPES_VERSION Extraction:
Usingtail -n 1after the grep forHeader *v...ResponseHeaderensures that only one (presumably the latest) version is picked up, avoiding the issue of double results.Suggestion: Consider adding an inline comment explaining why
tail -n 1is needed (i.e. due to multiple protocol header matches in newer versions) to assist future maintainers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/generate_grpc_service_handlers.sh(10 hunks)
🔇 Additional comments (15)
scripts/generate_grpc_service_handlers.sh (15)
8-9: New Templates for Service Methods:
The introduction ofSERVER_P2P_METHOD_TEMPLATEandSERVER_LOCAL_ON_CHAIN_TEMPLATEclearly separates the templates for P2P and local on-chain service methods. This lays the groundwork for more modular code generation.
11-12: Updated Output Path Variables:
The script now definesP2P_OUTPATHandLOCAL_OUTPATH. Although both are currently set to the same directory, this change facilitates future separation if needed.
14-16: Registration File Path Updates:
The variablesREGISTER_SERVICES_SERVER_FILE,REGISTER_SERVICES_CLIENT_FILE, andUNMARSHALLING_FILEhave been updated to use the newP2P_OUTPATH. This change aligns the generated file locations with the new routing logic.
51-53: EMPTY_IMPORT Parameter Replacement and Reset:
Adding the replacement for{{EMPTY_IMPORT}}in the general parameter string and subsequently resettingEMPTY_IMPORTis a neat way to handle conditional imports (e.g., for empty responses). Just ensure that the variable is set appropriately before it’s used in each template generation sequence.
112-119: Local On-Chain Service Branch Adjustments:
In the block handling local on-chain services, the server file is generated inLOCAL_OUTPATHand then, within the method loop, theSERVER_LOCAL_ON_CHAIN_TEMPLATEis used. This clearly distinguishes local on-chain generation from the P2P branch. The use of the defaultSERVER_TEMPLATEfor the initial server file (outside the method loop) appears to be intentional.
135-139: Local On-Chain Server Method Template Usage:
Inside the local on-chain branch’s loop, the script copies fromSERVER_LOCAL_ON_CHAIN_TEMPLATErather than the regular server template. This ensures that the generated methods incorporate any variation specific to local on-chain services.
148-153: Service Registration – Server Side Update:
The functiongenerate_register_services_servernow starts with a generated code notice using the{{GENERATOR}}placeholder. This stylistic update is consistent with the client registration function and looks good.
170-177: Service Registration – Client Side Update:
Similarly, the updated generated comment ingenerate_register_services_clientaligns with the overall changes in the script. The change improves consistency and traceability of generated files.
200-207: Unmarshalling Generation Update:
The update at the beginning of thegenerate_unmarshallingfunction (new echo with generated code notice) is minor but helps maintain consistency across all generated files.
233-236: Output Directory Cleanup and Creation:
The script now explicitly removes and recreates both the P2P and local on-chain output directories. This ensures a clean slate before file generation.Note: Since both paths currently point to the same directory, double-cleanup might be redundant—but it’s acceptable given the future possibility of separating these directories.
300-301: File Filtering Early Continue:
The addition ofcontinuewhen a file does not match the optional filter is a clear and effective way to skip unnecessary processing.
347-348: Enhanced Logging for Service and Tags:
The updated echo statements that log the service name and tagging information (type, routing, on-chain) are useful for debugging and transparency during service processing.
397-400: Registration Array Population for P2P Routing:
Appending the service version string toSERVICES_TO_REGISTERand updatingPROTO_INCLUDES_FOR_UNMARSHALLINGbased on P2P routing is well implemented. This ensures that only the relevant services are registered for P2P handling.
405-406: Generation of Registration Files:
The calls togenerate_register_services_serverandgenerate_register_services_clientwith the newly updated file paths reflect the changes in service handler generation and are correctly placed.
410-411: Running gofumpt on Generated Files:
Invoking gofumpt on both$P2P_OUTPATHand$LOCAL_OUTPATHensures that all generated code is formatted consistently. This is a good final step in the generation process.
04b2099 to
b5efad7
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
scripts/generate_grpc_service_handlers.sh (1)
419-419:⚠️ Potential issueFix the common types version extraction.
The current version extraction might give incorrect results. Based on past review comments, this needs the
tail -n 1fix to handle newer protocol versions correctly.Apply the suggested fix:
-COMMON_TYPES_VERSION=$(grep -oP '(?<=Header \*v)\d+(?=\.ResponseHeader)' "$pb_file" | tail -n 1) +COMMON_TYPES_VERSION=$(grep -oP '(?<=Header \*v)\d+(?=\.ResponseHeader)' "$pb_file" | sort -u | tail -n 1)
🧹 Nitpick comments (9)
internal/messaging/response_handler.go (1)
118-118: Would you like help implementing the TODOs?I notice two TODOs that need implementation:
- Adding support for
bookv3.MintResponse- Adding logic for processing request messages, starting with cancellation messages
I can help generate the implementation for either of these TODOs. Would you like me to:
- Generate the case for handling
bookv3.MintResponseinProcessResponseMessage?- Generate the initial implementation for handling cancellation messages in
ProcessRequestMessage?Also applies to: 127-130
internal/messaging/processor.go (2)
274-283: Consider adding error documentation for ProcessIncomingLocalMessage.While the implementation is correct, it would be beneficial to document the possible error types that can be returned by this method.
Add error documentation:
+// ProcessIncomingLocalMessage processes a local message and returns the response. +// Returns an error if: +// - message processing fails +// - response generation fails func (p *messageProcessor) ProcessIncomingLocalMessage( ctx context.Context, msg *types.Message, ) (*types.Message, error) {
285-301: Consider adding context timeout for local message processing.The method
ProcessIncomingLocalMessageAndGetResponseshould consider implementing a timeout mechanism similar toSendRequestMessageto prevent long-running operations.Consider adding timeout handling:
func (p *messageProcessor) ProcessIncomingLocalMessageAndGetResponse( ctx context.Context, requestMsg *types.Message, ) (context.Context, *types.Message, error) { + ctx, cancel := context.WithTimeout(ctx, p.responseTimeout) + defer cancel() requestMsg.Metadata.Stamp(fmt.Sprintf("%s-%s", p.Checkpoint(), "request"))scripts/generate_grpc_service_handlers.sh (1)
11-12: Consider separating P2P and LOCAL output paths.Currently, both P2P and LOCAL services output to the same directory. While this works, it might be worth considering separate directories for better organization.
Consider using distinct directories:
-P2P_OUTPATH="internal/rpc/generated" -LOCAL_OUTPATH="internal/rpc/generated" +P2P_OUTPATH="internal/rpc/generated/p2p" +LOCAL_OUTPATH="internal/rpc/generated/local"examples/rpc/partner-plugin/server.go (5)
103-279: Double-check extremely large year values.The usage of
time.Date(20024, …)appears to be a placeholder far in the future. Verify whether this is intentional or an inadvertent typo.
378-435: Validate placeholder date usage.Again,
20024is used as the year in several places. If these are just demo values, consider using more realistic or clearly “dummy” year references to avoid confusion.
494-782: Consider omitting or clarifying RetailerCode and SubSupplierCode fields.Sam suggests these fields can be left empty when unused. Including them with identical values (e.g.,
"BAW") may cause confusion in real-world scenarios. If they serve no practical purpose, consider removing or leaving them as empty strings:- RetailerCode: "BAW", - SubSupplierCode: "BAW", + // RetailerCode: "", + // SubSupplierCode: "",
784-928: Avoid “UNSPECIFIED” product codes if a meaningful type is known.Similar to TransportSearch, consider removing or replacing
"PRODUCT_CODE_TYPE_UNSPECIFIED"for more clarity. Operators or integrators might rely on that code type for filtering. If it is indeed unknown, explicitly documenting it can avoid confusion.
930-1104: Replace placeholder image URLs and vector labels.The code references a sample URL (
https://camino.network/...) with a TODO comment. If a real resource is not available, mention clearly in documentation that this is a mock or fill in with an accessible placeholder resource.Do you want me to open an issue for tracking updates to the seat map images or generate some alternative placeholders for demonstration?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (137)
go.sumis excluded by!**/*.suminternal/rpc/generated/accommodationv1_AccommodationProductInfoService_AccommodationProductInfo_server_method.gois excluded by!**/generated/**internal/rpc/generated/accommodationv1_AccommodationProductInfoService_client.gois excluded by!**/generated/**internal/rpc/generated/accommodationv1_AccommodationProductInfoService_server.gois excluded by!**/generated/**internal/rpc/generated/accommodationv1_AccommodationProductListService_AccommodationProductList_server_method.gois excluded by!**/generated/**internal/rpc/generated/accommodationv1_AccommodationProductListService_client.gois excluded by!**/generated/**internal/rpc/generated/accommodationv1_AccommodationProductListService_server.gois excluded by!**/generated/**internal/rpc/generated/accommodationv1_AccommodationSearchService_AccommodationSearch_server_method.gois excluded by!**/generated/**internal/rpc/generated/accommodationv1_AccommodationSearchService_client.gois excluded by!**/generated/**internal/rpc/generated/accommodationv1_AccommodationSearchService_server.gois excluded by!**/generated/**internal/rpc/generated/accommodationv2_AccommodationProductInfoService_AccommodationProductInfo_server_method.gois excluded by!**/generated/**internal/rpc/generated/accommodationv2_AccommodationProductInfoService_client.gois excluded by!**/generated/**internal/rpc/generated/accommodationv2_AccommodationProductInfoService_server.gois excluded by!**/generated/**internal/rpc/generated/accommodationv2_AccommodationProductListService_AccommodationProductList_server_method.gois excluded by!**/generated/**internal/rpc/generated/accommodationv2_AccommodationProductListService_client.gois excluded by!**/generated/**internal/rpc/generated/accommodationv2_AccommodationProductListService_server.gois excluded by!**/generated/**internal/rpc/generated/accommodationv2_AccommodationSearchService_AccommodationSearch_server_method.gois excluded by!**/generated/**internal/rpc/generated/accommodationv2_AccommodationSearchService_client.gois excluded by!**/generated/**internal/rpc/generated/accommodationv2_AccommodationSearchService_server.gois excluded by!**/generated/**internal/rpc/generated/accommodationv3_AccommodationProductInfoService_AccommodationProductInfo_client_method.gois excluded by!**/generated/**internal/rpc/generated/accommodationv3_AccommodationProductInfoService_AccommodationProductInfo_server_method.gois excluded by!**/generated/**internal/rpc/generated/accommodationv3_AccommodationProductInfoService_client.gois excluded by!**/generated/**internal/rpc/generated/accommodationv3_AccommodationProductInfoService_server.gois excluded by!**/generated/**internal/rpc/generated/accommodationv3_AccommodationProductListService_AccommodationProductList_client_method.gois excluded by!**/generated/**internal/rpc/generated/accommodationv3_AccommodationProductListService_AccommodationProductList_server_method.gois excluded by!**/generated/**internal/rpc/generated/accommodationv3_AccommodationProductListService_client.gois excluded by!**/generated/**internal/rpc/generated/accommodationv3_AccommodationProductListService_server.gois excluded by!**/generated/**internal/rpc/generated/accommodationv3_AccommodationSearchService_AccommodationSearch_client_method.gois excluded by!**/generated/**internal/rpc/generated/accommodationv3_AccommodationSearchService_AccommodationSearch_server_method.gois excluded by!**/generated/**internal/rpc/generated/accommodationv3_AccommodationSearchService_client.gois excluded by!**/generated/**internal/rpc/generated/accommodationv3_AccommodationSearchService_server.gois excluded by!**/generated/**internal/rpc/generated/activityv1_ActivityProductInfoService_ActivityProductInfo_server_method.gois excluded by!**/generated/**internal/rpc/generated/activityv1_ActivityProductInfoService_client.gois excluded by!**/generated/**internal/rpc/generated/activityv1_ActivityProductInfoService_server.gois excluded by!**/generated/**internal/rpc/generated/activityv1_ActivityProductListService_ActivityProductList_server_method.gois excluded by!**/generated/**internal/rpc/generated/activityv1_ActivityProductListService_client.gois excluded by!**/generated/**internal/rpc/generated/activityv1_ActivityProductListService_server.gois excluded by!**/generated/**internal/rpc/generated/activityv1_ActivitySearchService_ActivitySearch_server_method.gois excluded by!**/generated/**internal/rpc/generated/activityv1_ActivitySearchService_client.gois excluded by!**/generated/**internal/rpc/generated/activityv1_ActivitySearchService_server.gois excluded by!**/generated/**internal/rpc/generated/activityv2_ActivityProductInfoService_ActivityProductInfo_server_method.gois excluded by!**/generated/**internal/rpc/generated/activityv2_ActivityProductInfoService_client.gois excluded by!**/generated/**internal/rpc/generated/activityv2_ActivityProductInfoService_server.gois excluded by!**/generated/**internal/rpc/generated/activityv2_ActivityProductListService_ActivityProductList_server_method.gois excluded by!**/generated/**internal/rpc/generated/activityv2_ActivityProductListService_client.gois excluded by!**/generated/**internal/rpc/generated/activityv2_ActivityProductListService_server.gois excluded by!**/generated/**internal/rpc/generated/activityv2_ActivitySearchService_ActivitySearch_server_method.gois excluded by!**/generated/**internal/rpc/generated/activityv2_ActivitySearchService_client.gois excluded by!**/generated/**internal/rpc/generated/activityv2_ActivitySearchService_server.gois excluded by!**/generated/**internal/rpc/generated/activityv3_ActivityProductInfoService_ActivityProductInfo_client_method.gois excluded by!**/generated/**internal/rpc/generated/activityv3_ActivityProductInfoService_ActivityProductInfo_server_method.gois excluded by!**/generated/**internal/rpc/generated/activityv3_ActivityProductInfoService_client.gois excluded by!**/generated/**internal/rpc/generated/activityv3_ActivityProductInfoService_server.gois excluded by!**/generated/**internal/rpc/generated/activityv3_ActivityProductListService_ActivityProductList_client_method.gois excluded by!**/generated/**internal/rpc/generated/activityv3_ActivityProductListService_ActivityProductList_server_method.gois excluded by!**/generated/**internal/rpc/generated/activityv3_ActivityProductListService_client.gois excluded by!**/generated/**internal/rpc/generated/activityv3_ActivityProductListService_server.gois excluded by!**/generated/**internal/rpc/generated/activityv3_ActivitySearchService_ActivitySearch_client_method.gois excluded by!**/generated/**internal/rpc/generated/activityv3_ActivitySearchService_ActivitySearch_server_method.gois excluded by!**/generated/**internal/rpc/generated/activityv3_ActivitySearchService_client.gois excluded by!**/generated/**internal/rpc/generated/activityv3_ActivitySearchService_server.gois excluded by!**/generated/**internal/rpc/generated/bookv1_MintService_Mint_server_method.gois excluded by!**/generated/**internal/rpc/generated/bookv1_MintService_client.gois excluded by!**/generated/**internal/rpc/generated/bookv1_MintService_server.gois excluded by!**/generated/**internal/rpc/generated/bookv1_ValidationService_Validation_server_method.gois excluded by!**/generated/**internal/rpc/generated/bookv1_ValidationService_client.gois excluded by!**/generated/**internal/rpc/generated/bookv1_ValidationService_server.gois excluded by!**/generated/**internal/rpc/generated/bookv2_MintService_Mint_server_method.gois excluded by!**/generated/**internal/rpc/generated/bookv2_MintService_client.gois excluded by!**/generated/**internal/rpc/generated/bookv2_MintService_server.gois excluded by!**/generated/**internal/rpc/generated/bookv2_ValidationService_Validation_server_method.gois excluded by!**/generated/**internal/rpc/generated/bookv2_ValidationService_client.gois excluded by!**/generated/**internal/rpc/generated/bookv2_ValidationService_server.gois excluded by!**/generated/**internal/rpc/generated/bookv3_MintService_Mint_client_method.gois excluded by!**/generated/**internal/rpc/generated/bookv3_MintService_Mint_server_method.gois excluded by!**/generated/**internal/rpc/generated/bookv3_MintService_client.gois excluded by!**/generated/**internal/rpc/generated/bookv3_MintService_server.gois excluded by!**/generated/**internal/rpc/generated/bookv3_ValidationService_Validation_client_method.gois excluded by!**/generated/**internal/rpc/generated/bookv3_ValidationService_Validation_server_method.gois excluded by!**/generated/**internal/rpc/generated/bookv3_ValidationService_client.gois excluded by!**/generated/**internal/rpc/generated/bookv3_ValidationService_server.gois excluded by!**/generated/**internal/rpc/generated/infov1_CountryEntryRequirementsService_CountryEntryRequirements_server_method.gois excluded by!**/generated/**internal/rpc/generated/infov1_CountryEntryRequirementsService_client.gois excluded by!**/generated/**internal/rpc/generated/infov1_CountryEntryRequirementsService_server.gois excluded by!**/generated/**internal/rpc/generated/infov2_CountryEntryRequirementsService_CountryEntryRequirements_server_method.gois excluded by!**/generated/**internal/rpc/generated/infov2_CountryEntryRequirementsService_client.gois excluded by!**/generated/**internal/rpc/generated/infov2_CountryEntryRequirementsService_server.gois excluded by!**/generated/**internal/rpc/generated/insurancev1_InsuranceProductInfoService_InsuranceProductInfo_server_method.gois excluded by!**/generated/**internal/rpc/generated/insurancev1_InsuranceProductInfoService_client.gois excluded by!**/generated/**internal/rpc/generated/insurancev1_InsuranceProductInfoService_server.gois excluded by!**/generated/**internal/rpc/generated/insurancev1_InsuranceProductListService_InsuranceProductList_server_method.gois excluded by!**/generated/**internal/rpc/generated/insurancev1_InsuranceProductListService_client.gois excluded by!**/generated/**internal/rpc/generated/insurancev1_InsuranceProductListService_server.gois excluded by!**/generated/**internal/rpc/generated/insurancev1_InsuranceSearchService_InsuranceSearch_server_method.gois excluded by!**/generated/**internal/rpc/generated/insurancev1_InsuranceSearchService_client.gois excluded by!**/generated/**internal/rpc/generated/insurancev1_InsuranceSearchService_server.gois excluded by!**/generated/**internal/rpc/generated/pingv1_PingService_Ping_server_method.gois excluded by!**/generated/**internal/rpc/generated/pingv1_PingService_client.gois excluded by!**/generated/**internal/rpc/generated/pingv1_PingService_server.gois excluded by!**/generated/**internal/rpc/generated/register_client_services.gois excluded by!**/generated/**internal/rpc/generated/register_server_services.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv1_SeatMapAvailabilityService_SeatMapAvailability_server_method.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv1_SeatMapAvailabilityService_client.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv1_SeatMapAvailabilityService_server.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv1_SeatMapService_SeatMap_server_method.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv1_SeatMapService_client.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv1_SeatMapService_server.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv2_SeatMapAvailabilityService_SeatMapAvailability_server_method.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv2_SeatMapAvailabilityService_client.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv2_SeatMapAvailabilityService_server.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv2_SeatMapService_SeatMap_server_method.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv2_SeatMapService_client.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv2_SeatMapService_server.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv3_SeatMapAvailabilityService_SeatMapAvailability_client_method.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv3_SeatMapAvailabilityService_SeatMapAvailability_server_method.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv3_SeatMapAvailabilityService_client.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv3_SeatMapAvailabilityService_server.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv3_SeatMapService_SeatMap_client_method.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv3_SeatMapService_SeatMap_server_method.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv3_SeatMapService_client.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv3_SeatMapService_server.gois excluded by!**/generated/**internal/rpc/generated/transportv1_TransportSearchService_TransportSearch_server_method.gois excluded by!**/generated/**internal/rpc/generated/transportv1_TransportSearchService_client.gois excluded by!**/generated/**internal/rpc/generated/transportv1_TransportSearchService_server.gois excluded by!**/generated/**internal/rpc/generated/transportv2_TransportSearchService_TransportSearch_server_method.gois excluded by!**/generated/**internal/rpc/generated/transportv2_TransportSearchService_client.gois excluded by!**/generated/**internal/rpc/generated/transportv2_TransportSearchService_server.gois excluded by!**/generated/**internal/rpc/generated/transportv3_TransportProductListService_TransportProductList_client_method.gois excluded by!**/generated/**internal/rpc/generated/transportv3_TransportProductListService_TransportProductList_server_method.gois excluded by!**/generated/**internal/rpc/generated/transportv3_TransportProductListService_client.gois excluded by!**/generated/**internal/rpc/generated/transportv3_TransportProductListService_server.gois excluded by!**/generated/**internal/rpc/generated/transportv3_TransportSearchService_TransportSearch_client_method.gois excluded by!**/generated/**internal/rpc/generated/transportv3_TransportSearchService_TransportSearch_server_method.gois excluded by!**/generated/**internal/rpc/generated/transportv3_TransportSearchService_client.gois excluded by!**/generated/**internal/rpc/generated/transportv3_TransportSearchService_server.gois excluded by!**/generated/**internal/rpc/generated/unmarshal.gois excluded by!**/generated/**tests/e2e/bot/generated/client.gois excluded by!**/generated/**
📒 Files selected for processing (21)
examples/rpc/partner-plugin/server.go(33 hunks)go.mod(2 hunks)internal/app/app.go(1 hunks)internal/messaging/mint_v1.go(1 hunks)internal/messaging/mint_v2.go(1 hunks)internal/messaging/noop_response_handler.go(1 hunks)internal/messaging/processor.go(4 hunks)internal/messaging/processor_test.go(2 hunks)internal/messaging/response_handler.go(2 hunks)internal/rpc/rpc.go(1 hunks)internal/rpc/server/server.go(5 hunks)pp-mock/handlers/accommodation/v1/accommodation_product_list.go(1 hunks)pp-mock/handlers/accommodation/v1/accommodation_search.go(1 hunks)pp-mock/handlers/accommodation/v2/accommodation_product_list.go(1 hunks)pp-mock/handlers/accommodation/v2/accommodation_search.go(1 hunks)scripts/build.sh(0 hunks)scripts/generate_grpc_service_handlers.sh(8 hunks)templates/client.go.tpl(0 hunks)templates/server.go.tpl(1 hunks)templates/server_local_on_chain_method.go.tpl(1 hunks)templates/server_p2p_method.go.tpl(1 hunks)
💤 Files with no reviewable changes (2)
- scripts/build.sh
- templates/client.go.tpl
🚧 Files skipped from review as they are similar to previous changes (15)
- internal/messaging/mint_v1.go
- pp-mock/handlers/accommodation/v1/accommodation_search.go
- internal/messaging/mint_v2.go
- internal/messaging/processor_test.go
- templates/server_p2p_method.go.tpl
- internal/rpc/rpc.go
- pp-mock/handlers/accommodation/v1/accommodation_product_list.go
- internal/messaging/noop_response_handler.go
- internal/app/app.go
- templates/server.go.tpl
- pp-mock/handlers/accommodation/v2/accommodation_product_list.go
- templates/server_local_on_chain_method.go.tpl
- pp-mock/handlers/accommodation/v2/accommodation_search.go
- go.mod
- internal/rpc/server/server.go
🧰 Additional context used
🧠 Learnings (1)
examples/rpc/partner-plugin/server.go (1)
Learnt from: VjeraTurk
PR: chain4travel/camino-messenger-bot#89
File: pp-mock/handlers/accommodation/v3/accommodation_search.go:148-160
Timestamp: 2025-01-22T09:22:31.686Z
Learning: The AccommodationSearch service in v3 intentionally reuses the AccommodationSearchResult type from v2 as per the protocol definition. This is not a version compatibility issue.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Unit Tests
- GitHub Check: Static Analysis
🔇 Additional comments (9)
internal/messaging/response_handler.go (1)
49-50: LGTM! Interface extension aligns with PR objectives.The new
ProcessRequestMessagemethod is a well-structured addition to theResponseHandlerinterface, enabling the processing of incoming requests with appropriate error handling.internal/messaging/processor.go (2)
53-55: Interface changes reflect new service routing types.The MessageProcessor interface has been updated to handle both P2P and local messages separately, which aligns well with the PR's objective of distinguishing between different service types.
295-295: Address the TODO comment about error handling.There's an unresolved TODO comment about error handling placement. This needs to be addressed to ensure proper error handling flow.
This TODO was previously flagged by Noctunus. Please clarify the error handling strategy - should errors be added to the response header earlier in the process?
scripts/generate_grpc_service_handlers.sh (1)
7-12:✅ Verification successful
Verify if server.go.tpl is still needed.
Based on past review comments, there's a TODO to check if
server.go.tplis no longer used and needs to be deleted.Run this script to check for template usage:
🏁 Script executed:
#!/bin/bash # Check if server.go.tpl is referenced anywhere else in the codebase rg -l "server\.go\.tpl" --type-add 'script:*.sh' --type scriptLength of output: 106
Review Update:
server.go.tplValidatedThe search confirms that the
server.go.tpltemplate is still actively referenced inscripts/generate_grpc_service_handlers.sh, so it is needed as is. No further action is required on this front.examples/rpc/partner-plugin/server.go (5)
15-16: Consolidate and verify new v3 imports.These imports for v3 references are clear. However, ensure that all v2 references are indeed still required; otherwise, consider removing them to keep dependencies minimal and consistent.
Also applies to: 25-26, 28-30, 38-39, 42-42
58-71: Confirm correct embedding of v3 interfaces alongside v2.Embedding multiple v3 service servers in
partnerPluginwhile still retaining v2 references is acceptable if you intend to support both simultaneously. Otherwise, consider migrating all relevant services to v3 for consistency.
81-84: Use consistent success headers.Adding the success header in the validation response is appropriate. Just remember to handle failure scenarios consistently throughout the code if you anticipate errors or partial successes.
1106-1142: SeatMapAvailability logic looks consistent.The seat availability structure is coherent and matches the seat map pattern. No critical issues discovered.
1316-1326: v3 service registration alignment.All newly introduced v3 services are properly registered. Make sure you also deregister or remove older versions later if they’re no longer supported.
b5efad7 to
e3542ab
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
examples/rpc/partner-plugin/server.go (1)
138-139:⚠️ Potential issueFix incorrect year in date.
The date uses year 20024 which appears to be a typo and should likely be 2024.
Apply this diff to fix the year:
- StartDatetime: timestamppb.New(time.Date(20024, 9, 20, 11, 0o0, 0, 0, time.UTC)), // summary.start - EndDatetime: timestamppb.New(time.Date(20024, 9, 20, 12, 0o0, 0, 0, time.UTC)), + StartDatetime: timestamppb.New(time.Date(2024, 9, 20, 11, 0, 0, 0, time.UTC)), // summary.start + EndDatetime: timestamppb.New(time.Date(2024, 9, 20, 12, 0, 0, 0, time.UTC)),
♻️ Duplicate comments (1)
scripts/generate_grpc_service_handlers.sh (1)
389-401: 🛠️ Refactor suggestionImprove service tag extraction robustness.
The current tag extraction logic assumes a specific format and order. It should be more robust to handle variations in tag format and missing tags.
Apply this diff to improve the tag extraction:
-# Extract the annotation line -TAGS_LINE=$(grep -E '@custom:cmp-service' "$file") - -# Use shell parameter expansion to extract values -SERVICE_TYPE=${TAGS_LINE#*type:} -SERVICE_TYPE=${SERVICE_TYPE%% *} - -ROUTING=${TAGS_LINE#*routing:} -ROUTING=${ROUTING%% *} - -ON_CHAIN=${TAGS_LINE#*on-chain:} -ON_CHAIN=${ON_CHAIN%%[[:space:]]*} +# Extract tags using grep with specific patterns +SERVICE_TYPE=$(grep -E '@custom:cmp-service.*type:' "$file" | grep -oP 'type:\K[^ ]+' || echo "unknown") +ROUTING=$(grep -E '@custom:cmp-service.*routing:' "$file" | grep -oP 'routing:\K[^ ]+' || echo "p2p") +ON_CHAIN=$(grep -E '@custom:cmp-service.*on-chain:' "$file" | grep -oP 'on-chain:\K[^ ]+' || echo "false") + +# Validate extracted values +if [[ "$SERVICE_TYPE" == "unknown" ]]; then + echo "⚠️ Warning: Missing service type tag in $file" +fi
🧹 Nitpick comments (7)
examples/rpc/partner-plugin/server.go (3)
15-28: Consider upgrading remaining v2 services to v3 for consistency.The codebase shows a mix of v2 and v3 service versions. While some services like
accommodation,activity,seat_map, andtransporthave been upgraded to v3, others likebook,info, andpartnerare still using v2. Consider planning the migration of the remaining services to v3 for consistency.Also applies to: 52-70
1245-1247: Implement missing insurance service examples.The insurance service implementations are incomplete with TODO comments. This could affect testing and documentation.
Would you like me to help implement these example responses based on the protocol definitions?
Also applies to: 1261-1263, 1277-1279
1308-1350: Add graceful shutdown handling.The server implementation could benefit from graceful shutdown handling to ensure clean termination of ongoing requests.
Consider implementing graceful shutdown:
func main() { + ctx, stop := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM) + defer stop() + grpcServer := grpc.NewServer() // ... server setup ... - grpcServer.Serve(lis) + go func() { + if err := grpcServer.Serve(lis); err != nil { + log.Fatalf("Failed to serve: %v", err) + } + }() + + <-ctx.Done() + log.Println("Shutting down gracefully...") + grpcServer.GracefulStop() }internal/messaging/processor.go (3)
274-283: Improve error handling in the wrapper function.The function could benefit from more descriptive error handling to help with debugging and troubleshooting.
Apply this diff to enhance error handling:
func (p *messageProcessor) ProcessIncomingLocalMessage( ctx context.Context, msg *types.Message, ) (*types.Message, error) { _, responseMsg, err := p.ProcessIncomingLocalMessageAndGetResponse(ctx, msg) if err != nil { - return responseMsg, err + return responseMsg, fmt.Errorf("failed to process local message: %w", err) } return responseMsg, nil }
293-299: Improve logging to avoid exposing sensitive data.The current logging of the entire response message content could expose sensitive data. Consider logging only non-sensitive metadata or using debug level for full content.
Apply this diff to improve logging:
- p.logger.Infof("responseMsg before processing %v", responseMsg.Content) + p.logger.Debugf("Processing message with ID: %s", responseMsg.Metadata.RequestID) if err := p.responseHandler.ProcessRequestMessage(ctx, responseMsg, requestMsg.Content); err != nil { p.responseHandler.AddErrorToResponseHeader(responseMsg.Content, err.Error()) return ctx, responseMsg, fmt.Errorf("failed to process request message: %w", err) } - p.logger.Infof("responseMsg after processing %v", responseMsg.Content) + p.logger.Debugf("Completed processing message with ID: %s", responseMsg.Metadata.RequestID)
285-301: Enhance error handling with context and validation.The function could benefit from input validation and more context in error messages.
Apply this diff to improve error handling:
func (p *messageProcessor) ProcessIncomingLocalMessageAndGetResponse( ctx context.Context, requestMsg *types.Message, ) (context.Context, *types.Message, error) { + if requestMsg == nil { + return ctx, nil, fmt.Errorf("request message cannot be nil") + } + if requestMsg.Content == nil { + return ctx, nil, fmt.Errorf("request message content cannot be nil") + } + requestMsg.Metadata.Stamp(fmt.Sprintf("%s-%s", p.Checkpoint(), "request")) responseMsg := &types.Message{ Metadata: requestMsg.Metadata, } p.logger.Debugf("Processing message with ID: %s", responseMsg.Metadata.RequestID) if err := p.responseHandler.ProcessRequestMessage(ctx, responseMsg, requestMsg.Content); err != nil { p.responseHandler.AddErrorToResponseHeader(responseMsg.Content, err.Error()) - return ctx, responseMsg, fmt.Errorf("failed to process request message: %w", err) + return ctx, responseMsg, fmt.Errorf("failed to process local request message (ID: %s): %w", + requestMsg.Metadata.RequestID, err) } p.logger.Debugf("Completed processing message with ID: %s", responseMsg.Metadata.RequestID) return ctx, responseMsg, nil }scripts/generate_grpc_service_handlers.sh (1)
20-20: Clarify and track the TODO about claim imports.The TODO comment about claim imports using
typevinstead oftypev1needs more context and should be tracked.Would you like me to help create an issue to track this TODO? Please provide more context about the version mismatch problem.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (137)
go.sumis excluded by!**/*.suminternal/rpc/generated/accommodationv1_AccommodationProductInfoService_AccommodationProductInfo_server_method.gois excluded by!**/generated/**internal/rpc/generated/accommodationv1_AccommodationProductInfoService_client.gois excluded by!**/generated/**internal/rpc/generated/accommodationv1_AccommodationProductInfoService_server.gois excluded by!**/generated/**internal/rpc/generated/accommodationv1_AccommodationProductListService_AccommodationProductList_server_method.gois excluded by!**/generated/**internal/rpc/generated/accommodationv1_AccommodationProductListService_client.gois excluded by!**/generated/**internal/rpc/generated/accommodationv1_AccommodationProductListService_server.gois excluded by!**/generated/**internal/rpc/generated/accommodationv1_AccommodationSearchService_AccommodationSearch_server_method.gois excluded by!**/generated/**internal/rpc/generated/accommodationv1_AccommodationSearchService_client.gois excluded by!**/generated/**internal/rpc/generated/accommodationv1_AccommodationSearchService_server.gois excluded by!**/generated/**internal/rpc/generated/accommodationv2_AccommodationProductInfoService_AccommodationProductInfo_server_method.gois excluded by!**/generated/**internal/rpc/generated/accommodationv2_AccommodationProductInfoService_client.gois excluded by!**/generated/**internal/rpc/generated/accommodationv2_AccommodationProductInfoService_server.gois excluded by!**/generated/**internal/rpc/generated/accommodationv2_AccommodationProductListService_AccommodationProductList_server_method.gois excluded by!**/generated/**internal/rpc/generated/accommodationv2_AccommodationProductListService_client.gois excluded by!**/generated/**internal/rpc/generated/accommodationv2_AccommodationProductListService_server.gois excluded by!**/generated/**internal/rpc/generated/accommodationv2_AccommodationSearchService_AccommodationSearch_server_method.gois excluded by!**/generated/**internal/rpc/generated/accommodationv2_AccommodationSearchService_client.gois excluded by!**/generated/**internal/rpc/generated/accommodationv2_AccommodationSearchService_server.gois excluded by!**/generated/**internal/rpc/generated/accommodationv3_AccommodationProductInfoService_AccommodationProductInfo_client_method.gois excluded by!**/generated/**internal/rpc/generated/accommodationv3_AccommodationProductInfoService_AccommodationProductInfo_server_method.gois excluded by!**/generated/**internal/rpc/generated/accommodationv3_AccommodationProductInfoService_client.gois excluded by!**/generated/**internal/rpc/generated/accommodationv3_AccommodationProductInfoService_server.gois excluded by!**/generated/**internal/rpc/generated/accommodationv3_AccommodationProductListService_AccommodationProductList_client_method.gois excluded by!**/generated/**internal/rpc/generated/accommodationv3_AccommodationProductListService_AccommodationProductList_server_method.gois excluded by!**/generated/**internal/rpc/generated/accommodationv3_AccommodationProductListService_client.gois excluded by!**/generated/**internal/rpc/generated/accommodationv3_AccommodationProductListService_server.gois excluded by!**/generated/**internal/rpc/generated/accommodationv3_AccommodationSearchService_AccommodationSearch_client_method.gois excluded by!**/generated/**internal/rpc/generated/accommodationv3_AccommodationSearchService_AccommodationSearch_server_method.gois excluded by!**/generated/**internal/rpc/generated/accommodationv3_AccommodationSearchService_client.gois excluded by!**/generated/**internal/rpc/generated/accommodationv3_AccommodationSearchService_server.gois excluded by!**/generated/**internal/rpc/generated/activityv1_ActivityProductInfoService_ActivityProductInfo_server_method.gois excluded by!**/generated/**internal/rpc/generated/activityv1_ActivityProductInfoService_client.gois excluded by!**/generated/**internal/rpc/generated/activityv1_ActivityProductInfoService_server.gois excluded by!**/generated/**internal/rpc/generated/activityv1_ActivityProductListService_ActivityProductList_server_method.gois excluded by!**/generated/**internal/rpc/generated/activityv1_ActivityProductListService_client.gois excluded by!**/generated/**internal/rpc/generated/activityv1_ActivityProductListService_server.gois excluded by!**/generated/**internal/rpc/generated/activityv1_ActivitySearchService_ActivitySearch_server_method.gois excluded by!**/generated/**internal/rpc/generated/activityv1_ActivitySearchService_client.gois excluded by!**/generated/**internal/rpc/generated/activityv1_ActivitySearchService_server.gois excluded by!**/generated/**internal/rpc/generated/activityv2_ActivityProductInfoService_ActivityProductInfo_server_method.gois excluded by!**/generated/**internal/rpc/generated/activityv2_ActivityProductInfoService_client.gois excluded by!**/generated/**internal/rpc/generated/activityv2_ActivityProductInfoService_server.gois excluded by!**/generated/**internal/rpc/generated/activityv2_ActivityProductListService_ActivityProductList_server_method.gois excluded by!**/generated/**internal/rpc/generated/activityv2_ActivityProductListService_client.gois excluded by!**/generated/**internal/rpc/generated/activityv2_ActivityProductListService_server.gois excluded by!**/generated/**internal/rpc/generated/activityv2_ActivitySearchService_ActivitySearch_server_method.gois excluded by!**/generated/**internal/rpc/generated/activityv2_ActivitySearchService_client.gois excluded by!**/generated/**internal/rpc/generated/activityv2_ActivitySearchService_server.gois excluded by!**/generated/**internal/rpc/generated/activityv3_ActivityProductInfoService_ActivityProductInfo_client_method.gois excluded by!**/generated/**internal/rpc/generated/activityv3_ActivityProductInfoService_ActivityProductInfo_server_method.gois excluded by!**/generated/**internal/rpc/generated/activityv3_ActivityProductInfoService_client.gois excluded by!**/generated/**internal/rpc/generated/activityv3_ActivityProductInfoService_server.gois excluded by!**/generated/**internal/rpc/generated/activityv3_ActivityProductListService_ActivityProductList_client_method.gois excluded by!**/generated/**internal/rpc/generated/activityv3_ActivityProductListService_ActivityProductList_server_method.gois excluded by!**/generated/**internal/rpc/generated/activityv3_ActivityProductListService_client.gois excluded by!**/generated/**internal/rpc/generated/activityv3_ActivityProductListService_server.gois excluded by!**/generated/**internal/rpc/generated/activityv3_ActivitySearchService_ActivitySearch_client_method.gois excluded by!**/generated/**internal/rpc/generated/activityv3_ActivitySearchService_ActivitySearch_server_method.gois excluded by!**/generated/**internal/rpc/generated/activityv3_ActivitySearchService_client.gois excluded by!**/generated/**internal/rpc/generated/activityv3_ActivitySearchService_server.gois excluded by!**/generated/**internal/rpc/generated/bookv1_MintService_Mint_server_method.gois excluded by!**/generated/**internal/rpc/generated/bookv1_MintService_client.gois excluded by!**/generated/**internal/rpc/generated/bookv1_MintService_server.gois excluded by!**/generated/**internal/rpc/generated/bookv1_ValidationService_Validation_server_method.gois excluded by!**/generated/**internal/rpc/generated/bookv1_ValidationService_client.gois excluded by!**/generated/**internal/rpc/generated/bookv1_ValidationService_server.gois excluded by!**/generated/**internal/rpc/generated/bookv2_MintService_Mint_server_method.gois excluded by!**/generated/**internal/rpc/generated/bookv2_MintService_client.gois excluded by!**/generated/**internal/rpc/generated/bookv2_MintService_server.gois excluded by!**/generated/**internal/rpc/generated/bookv2_ValidationService_Validation_server_method.gois excluded by!**/generated/**internal/rpc/generated/bookv2_ValidationService_client.gois excluded by!**/generated/**internal/rpc/generated/bookv2_ValidationService_server.gois excluded by!**/generated/**internal/rpc/generated/bookv3_MintService_Mint_client_method.gois excluded by!**/generated/**internal/rpc/generated/bookv3_MintService_Mint_server_method.gois excluded by!**/generated/**internal/rpc/generated/bookv3_MintService_client.gois excluded by!**/generated/**internal/rpc/generated/bookv3_MintService_server.gois excluded by!**/generated/**internal/rpc/generated/bookv3_ValidationService_Validation_client_method.gois excluded by!**/generated/**internal/rpc/generated/bookv3_ValidationService_Validation_server_method.gois excluded by!**/generated/**internal/rpc/generated/bookv3_ValidationService_client.gois excluded by!**/generated/**internal/rpc/generated/bookv3_ValidationService_server.gois excluded by!**/generated/**internal/rpc/generated/infov1_CountryEntryRequirementsService_CountryEntryRequirements_server_method.gois excluded by!**/generated/**internal/rpc/generated/infov1_CountryEntryRequirementsService_client.gois excluded by!**/generated/**internal/rpc/generated/infov1_CountryEntryRequirementsService_server.gois excluded by!**/generated/**internal/rpc/generated/infov2_CountryEntryRequirementsService_CountryEntryRequirements_server_method.gois excluded by!**/generated/**internal/rpc/generated/infov2_CountryEntryRequirementsService_client.gois excluded by!**/generated/**internal/rpc/generated/infov2_CountryEntryRequirementsService_server.gois excluded by!**/generated/**internal/rpc/generated/insurancev1_InsuranceProductInfoService_InsuranceProductInfo_server_method.gois excluded by!**/generated/**internal/rpc/generated/insurancev1_InsuranceProductInfoService_client.gois excluded by!**/generated/**internal/rpc/generated/insurancev1_InsuranceProductInfoService_server.gois excluded by!**/generated/**internal/rpc/generated/insurancev1_InsuranceProductListService_InsuranceProductList_server_method.gois excluded by!**/generated/**internal/rpc/generated/insurancev1_InsuranceProductListService_client.gois excluded by!**/generated/**internal/rpc/generated/insurancev1_InsuranceProductListService_server.gois excluded by!**/generated/**internal/rpc/generated/insurancev1_InsuranceSearchService_InsuranceSearch_server_method.gois excluded by!**/generated/**internal/rpc/generated/insurancev1_InsuranceSearchService_client.gois excluded by!**/generated/**internal/rpc/generated/insurancev1_InsuranceSearchService_server.gois excluded by!**/generated/**internal/rpc/generated/pingv1_PingService_Ping_server_method.gois excluded by!**/generated/**internal/rpc/generated/pingv1_PingService_client.gois excluded by!**/generated/**internal/rpc/generated/pingv1_PingService_server.gois excluded by!**/generated/**internal/rpc/generated/register_client_services.gois excluded by!**/generated/**internal/rpc/generated/register_server_services.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv1_SeatMapAvailabilityService_SeatMapAvailability_server_method.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv1_SeatMapAvailabilityService_client.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv1_SeatMapAvailabilityService_server.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv1_SeatMapService_SeatMap_server_method.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv1_SeatMapService_client.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv1_SeatMapService_server.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv2_SeatMapAvailabilityService_SeatMapAvailability_server_method.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv2_SeatMapAvailabilityService_client.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv2_SeatMapAvailabilityService_server.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv2_SeatMapService_SeatMap_server_method.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv2_SeatMapService_client.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv2_SeatMapService_server.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv3_SeatMapAvailabilityService_SeatMapAvailability_client_method.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv3_SeatMapAvailabilityService_SeatMapAvailability_server_method.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv3_SeatMapAvailabilityService_client.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv3_SeatMapAvailabilityService_server.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv3_SeatMapService_SeatMap_client_method.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv3_SeatMapService_SeatMap_server_method.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv3_SeatMapService_client.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv3_SeatMapService_server.gois excluded by!**/generated/**internal/rpc/generated/transportv1_TransportSearchService_TransportSearch_server_method.gois excluded by!**/generated/**internal/rpc/generated/transportv1_TransportSearchService_client.gois excluded by!**/generated/**internal/rpc/generated/transportv1_TransportSearchService_server.gois excluded by!**/generated/**internal/rpc/generated/transportv2_TransportSearchService_TransportSearch_server_method.gois excluded by!**/generated/**internal/rpc/generated/transportv2_TransportSearchService_client.gois excluded by!**/generated/**internal/rpc/generated/transportv2_TransportSearchService_server.gois excluded by!**/generated/**internal/rpc/generated/transportv3_TransportProductListService_TransportProductList_client_method.gois excluded by!**/generated/**internal/rpc/generated/transportv3_TransportProductListService_TransportProductList_server_method.gois excluded by!**/generated/**internal/rpc/generated/transportv3_TransportProductListService_client.gois excluded by!**/generated/**internal/rpc/generated/transportv3_TransportProductListService_server.gois excluded by!**/generated/**internal/rpc/generated/transportv3_TransportSearchService_TransportSearch_client_method.gois excluded by!**/generated/**internal/rpc/generated/transportv3_TransportSearchService_TransportSearch_server_method.gois excluded by!**/generated/**internal/rpc/generated/transportv3_TransportSearchService_client.gois excluded by!**/generated/**internal/rpc/generated/transportv3_TransportSearchService_server.gois excluded by!**/generated/**internal/rpc/generated/unmarshal.gois excluded by!**/generated/**tests/e2e/bot/generated/client.gois excluded by!**/generated/**
📒 Files selected for processing (21)
examples/rpc/partner-plugin/server.go(33 hunks)go.mod(2 hunks)internal/app/app.go(1 hunks)internal/messaging/mint_v1.go(1 hunks)internal/messaging/mint_v2.go(1 hunks)internal/messaging/noop_response_handler.go(1 hunks)internal/messaging/processor.go(4 hunks)internal/messaging/processor_test.go(2 hunks)internal/messaging/response_handler.go(2 hunks)internal/rpc/rpc.go(1 hunks)internal/rpc/server/server.go(5 hunks)pp-mock/handlers/accommodation/v1/accommodation_product_list.go(1 hunks)pp-mock/handlers/accommodation/v1/accommodation_search.go(1 hunks)pp-mock/handlers/accommodation/v2/accommodation_product_list.go(1 hunks)pp-mock/handlers/accommodation/v2/accommodation_search.go(1 hunks)scripts/build.sh(0 hunks)scripts/generate_grpc_service_handlers.sh(8 hunks)templates/client.go.tpl(0 hunks)templates/server.go.tpl(1 hunks)templates/server_local_on_chain_method.go.tpl(1 hunks)templates/server_p2p_method.go.tpl(1 hunks)
💤 Files with no reviewable changes (2)
- scripts/build.sh
- templates/client.go.tpl
🚧 Files skipped from review as they are similar to previous changes (16)
- pp-mock/handlers/accommodation/v2/accommodation_product_list.go
- pp-mock/handlers/accommodation/v1/accommodation_product_list.go
- internal/app/app.go
- templates/server.go.tpl
- internal/messaging/mint_v1.go
- pp-mock/handlers/accommodation/v2/accommodation_search.go
- internal/messaging/mint_v2.go
- pp-mock/handlers/accommodation/v1/accommodation_search.go
- internal/rpc/rpc.go
- internal/rpc/server/server.go
- internal/messaging/response_handler.go
- templates/server_p2p_method.go.tpl
- internal/messaging/processor_test.go
- go.mod
- templates/server_local_on_chain_method.go.tpl
- internal/messaging/noop_response_handler.go
🧰 Additional context used
🧠 Learnings (1)
examples/rpc/partner-plugin/server.go (1)
Learnt from: VjeraTurk
PR: chain4travel/camino-messenger-bot#89
File: pp-mock/handlers/accommodation/v3/accommodation_search.go:148-160
Timestamp: 2025-01-22T09:22:31.686Z
Learning: The AccommodationSearch service in v3 intentionally reuses the AccommodationSearchResult type from v2 as per the protocol definition. This is not a version compatibility issue.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Unit Tests
- GitHub Check: Build docker image
- GitHub Check: E2E Tests
- GitHub Check: Static Analysis
🔇 Additional comments (5)
examples/rpc/partner-plugin/server.go (1)
556-556: Avoid using UNSPECIFIED product code types.Multiple instances of
PRODUCT_CODE_TYPE_UNSPECIFIEDare used in the code. Based on the past review comments, this should be avoided as it holds no meaning and should be treated as "not provided".Also applies to: 809-809, 893-893
internal/messaging/processor.go (2)
134-144: LGTM! Function renamed to better reflect its purpose.The function has been appropriately renamed from
ProcessIncomingMessagetoProcessIncomingP2PMessageto better indicate its specific role in handling P2P messages. The logic remains correct and unchanged.
295-295: Address the TODO comment about error handling.The TODO comment suggests uncertainty about when to call
AddErrorToResponseHeader. This needs to be resolved.scripts/generate_grpc_service_handlers.sh (2)
8-12: LGTM! Clear separation of templates for different routing types.The addition of separate templates for P2P and local on-chain methods improves code organization and maintainability.
419-419: LGTM! Version extraction issue fixed.The
tail -n 1fix has been applied to handle multiple version outputs correctly.
| "google.golang.org/grpc" | ||
| ) | ||
|
|
||
| const ( |
There was a problem hiding this comment.
why was it moved from client template to server template?
There was a problem hiding this comment.
@evlkht Because local CancellationService only has a server (no client) a hence would fail to have this.
There was a problem hiding this comment.
I believe we should keep client for local service
E.g. take a look at readiness rpc in bot
|
@VjeraTurk please cleanup PR from all leftovers, remove everything related to on-chain template if its not used in code in any way and also clean corresponding namings in code / script. |
| _ *types.Message, | ||
| _ protoreflect.ProtoMessage, | ||
| ) error { | ||
| // TODO: @VjeraTurk A the logic for processing these request messages |
There was a problem hiding this comment.
what is this? it doesn't look used
There was a problem hiding this comment.
The only service that demonstrates these changes is the CancellationService - separated from this PR. If I remove the function from here, I have to remove the ProcessIncomingLocalMessageAndGetResponse also, as it is not used, and this is the only place where the name of this function is most recently agreed up on. If I remove it from here, we will only be left with the outdated names on cancellation-operation branch.
Let me know your suggestion - if we really take the strict approach and strip down this branch from everything related to cancellation, how do we ensure it is available and correct later on?
| @@ -112,9 +115,22 @@ func (h *evmResponseHandler) ProcessResponseMessage( | |||
| h.processMintResponseV1(ctx, response) | |||
| case *bookv2.MintResponse: // distributor will post-process a mint request to buy the returned NFT | |||
| h.processMintResponseV2(ctx, response) | |||
| // TODO: @VjeraTurk Add bookv3.MintResponse | |||
There was a problem hiding this comment.
Yes, this comment should only be on cancellation-operation branch. I removed it.
e3542ab to
9a18b85
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
scripts/generate_grpc_service_handlers.sh (5)
61-69: P2P Client File Generation Block
The block for generating the P2P client file is clear and functions as intended. However, for improved robustness and to avoid potential issues with file paths that contain spaces, consider quoting file path variables (e.g.,"$CLIENT_TEMPLATE"and"$CLIENT_GEN_FILE") in thecpandsedcommands.
84-93: P2P Client Methods Generation
The loop generating client methods, including handling for methods that returnEmpty, is well implemented. As with the previous block, wrapping file path and variable references in quotes could further increase the script’s resilience.
128-155: Local On-chain Server Methods Generation
The logic for generating local on-chain server methods is cleanly separated and uses the appropriate$SERVER_LOCAL_ON_CHAIN_TEMPLATE. As with other file operations, consider quoting file path variables in commands such ascpandsedfor consistency and robustness.
304-308: Cleaning and Creating Output Directories
The commands for cleaning (rm -rf) and creating (mkdir -p) output directories use unquoted variables. To prevent potential issues with directory names that contain spaces, consider quoting these variables. For example:- rm -rf $P2P_OUTPATH - mkdir -p $P2P_OUTPATH + rm -rf "$P2P_OUTPATH" + mkdir -p "$P2P_OUTPATH"
389-401: Service Tags Extraction Method
The extraction of service tags from the annotation line using shell parameter expansion works in straightforward cases. However, given potential variability in annotation formatting, it might be more robust to extract these values with tools likegrep -oPand default fallback values. Consider refactoring this section if tag formatting becomes inconsistent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/generate_grpc_service_handlers.sh(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Unit Tests
- GitHub Check: Build docker image
- GitHub Check: E2E Tests
- GitHub Check: Static Analysis
🔇 Additional comments (10)
scripts/generate_grpc_service_handlers.sh (10)
8-9: New Template Variables for Service Handling
The introduction ofSERVER_P2P_METHOD_TEMPLATEandSERVER_LOCAL_ON_CHAIN_TEMPLATEclearly separates the logic for generating service handlers based on routing type. Ensure that the actual template files in thetemplatesdirectory match the expected structure for P2P and local on-chain services.
11-12: Output Directories for Service Handlers
BothP2P_OUTPATHandLOCAL_OUTPATHare currently set to the same directory (internal/rpc/generated). While this is acceptable for now, verify whether future changes require physically separate directories for P2P and local on-chain services.
15-17: Updated Registration and Unmarshalling Paths
The updates toREGISTER_SERVICES_SERVER_FILE,REGISTER_SERVICES_CLIENT_FILE, andUNMARSHALING_FILEnow reference${P2P_OUTPATH}. This is consistent with the changes for P2P routing. Please double-check that local on-chain services do not need separate paths or adjustments in these registration files.
52-53: Addition of EMPTY_IMPORT in Template Replacement
Adding the{{EMPTY_IMPORT}}substitution improves flexibility for handling scenarios where an empty import might be needed (for example, when the output is "Empty"). This enhances the robustness of the templating logic.
59-59: Resetting EMPTY_IMPORT
ResettingEMPTY_IMPORTto an empty string ensures that any previous value does not inadvertently affect futuresedsubstitutions.
95-101: P2P Server File Generation
Generating the primary P2P server file with the existingSERVER_TEMPLATEis correctly executed, and the corresponding template substitution is straightforward.
419-419: Extraction of COMMON_TYPES_VERSION
The extraction ofCOMMON_TYPES_VERSIONusinggrepwith a lookbehind and lookahead, followed bytail -n 1, ensures that the correct version is captured even if multiple matches occur. This approach effectively counters the double-output issue noted in previous discussions.
472-477: Template Generation Call for Client and Server Files
The construction ofPROTO_INCLUDEandGRPC_INCLUDEas well as the subsequent call togenerate_with_templatesis clear and aligns with the new generation logic for service handlers. Ensure that the constructed paths accurately reflect the latest SDK structure and output directory design.
483-487: Registration of P2P Services
Appending toSERVICES_TO_REGISTERandPROTO_INCLUDES_FOR_UNMARSHALLINGexclusively for P2P routing matches the design where only P2P services register for method unmarshalling. Verify whether local on-chain services may require analogous registration in the future.
497-498: Running gofumpt on Generated Code
Applyinggofumpton the generated directories ($P2P_OUTPATH,$LOCAL_OUTPATH, and$E2E_GEN_OUTPATH) ensures code formatting consistency. Confirm that the$FUMPTpath is valid and that this formatting step does not inadvertently alter any generated code syntax.
9a18b85 to
e43a2fa
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
examples/rpc/partner-plugin/server.go (1)
556-556: 🛠️ Refactor suggestionAvoid using UNSPECIFIED product code type.
As per previous feedback, UNSPECIFIED enum values should only be used when a field is not provided, not as an explicit value.
- Type: typesv2.ProductCodeType_PRODUCT_CODE_TYPE_UNSPECIFIED, + Type: typesv2.ProductCodeType_PRODUCT_CODE_TYPE_FLIGHT_NUMBER,scripts/generate_grpc_service_handlers.sh (1)
118-126: 🛠️ Refactor suggestion
⚠️ Potential issueIncorrect template variable substitution in P2P server method generation.
On line 126, the script substitutes{{TEMPLATE}}with$SERVER_METHOD_TEMPLATE, which is not defined for P2P methods. It should instead use$SERVER_P2P_METHOD_TEMPLATEto ensure the correct template is applied. Consider this diff:- sed -i -e "s#{{TEMPLATE}}#$SERVER_METHOD_TEMPLATE#g" "$METHOD_GEN_FILE" + sed -i -e "s#{{TEMPLATE}}#$SERVER_P2P_METHOD_TEMPLATE#g" "$METHOD_GEN_FILE"
🧹 Nitpick comments (7)
examples/rpc/partner-plugin/server.go (1)
1246-1247: Add implementation for insurance-related TODO comments.The insurance-related methods are missing implementations. This could affect the functionality of the insurance features.
Would you like me to help generate example implementations for these insurance-related methods?
Also applies to: 1262-1263, 1278-1279
scripts/generate_grpc_service_handlers.sh (6)
20-21: Note on TODO and blacklist configuration.
The TODO comment regarding the claim import type remains as an actionable item, and theDEFAULT_BLACKLISTnow explicitly excludes services such as “claim.” It is advisable to revisit the TODO when more context about the claim versioning issue is available.
61-93: P2P Client Code Generation Logic.
The client generation branch (lines 61–93) successfully creates both the client file and its associated methods using the provided templates. For improved robustness, consider quoting variables (e.g. in thecpandsedcommands) to avoid potential issues with word splitting.
304-309: Output Directories Cleanup and Preparation.
The script ensures a clean build state by removing and recreating both the P2P and local output directories. For extra safety, consider quoting the variable names (e.g.,"$P2P_OUTPATH").
323-347: Determining GOPATH Robustly.
The multi-step strategy to setGO_PATHincreases robustness. Enhancing the quoting in command substitutions and file tests can further improve portability across shells.
389-401: Service Tag Extraction Logic.
ExtractingSERVICE_TYPE,ROUTING, andON_CHAINvia shell parameter expansion is straightforward; however, this method may be brittle if the annotation format changes. Enhancing the extraction (for example, by using grep with Perl-compatible regex and default fallbacks) could improve robustness.
427-450: Method Extraction and Parameter Parsing.
The loop that processes method names and extracts their input/output types using grep and cut works as expected but is sensitive to formatting variations. A more robust parser might be considered in the long run.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (137)
go.sumis excluded by!**/*.suminternal/rpc/generated/accommodationv1_AccommodationProductInfoService_AccommodationProductInfo_server_method.gois excluded by!**/generated/**internal/rpc/generated/accommodationv1_AccommodationProductInfoService_client.gois excluded by!**/generated/**internal/rpc/generated/accommodationv1_AccommodationProductInfoService_server.gois excluded by!**/generated/**internal/rpc/generated/accommodationv1_AccommodationProductListService_AccommodationProductList_server_method.gois excluded by!**/generated/**internal/rpc/generated/accommodationv1_AccommodationProductListService_client.gois excluded by!**/generated/**internal/rpc/generated/accommodationv1_AccommodationProductListService_server.gois excluded by!**/generated/**internal/rpc/generated/accommodationv1_AccommodationSearchService_AccommodationSearch_server_method.gois excluded by!**/generated/**internal/rpc/generated/accommodationv1_AccommodationSearchService_client.gois excluded by!**/generated/**internal/rpc/generated/accommodationv1_AccommodationSearchService_server.gois excluded by!**/generated/**internal/rpc/generated/accommodationv2_AccommodationProductInfoService_AccommodationProductInfo_server_method.gois excluded by!**/generated/**internal/rpc/generated/accommodationv2_AccommodationProductInfoService_client.gois excluded by!**/generated/**internal/rpc/generated/accommodationv2_AccommodationProductInfoService_server.gois excluded by!**/generated/**internal/rpc/generated/accommodationv2_AccommodationProductListService_AccommodationProductList_server_method.gois excluded by!**/generated/**internal/rpc/generated/accommodationv2_AccommodationProductListService_client.gois excluded by!**/generated/**internal/rpc/generated/accommodationv2_AccommodationProductListService_server.gois excluded by!**/generated/**internal/rpc/generated/accommodationv2_AccommodationSearchService_AccommodationSearch_server_method.gois excluded by!**/generated/**internal/rpc/generated/accommodationv2_AccommodationSearchService_client.gois excluded by!**/generated/**internal/rpc/generated/accommodationv2_AccommodationSearchService_server.gois excluded by!**/generated/**internal/rpc/generated/accommodationv3_AccommodationProductInfoService_AccommodationProductInfo_client_method.gois excluded by!**/generated/**internal/rpc/generated/accommodationv3_AccommodationProductInfoService_AccommodationProductInfo_server_method.gois excluded by!**/generated/**internal/rpc/generated/accommodationv3_AccommodationProductInfoService_client.gois excluded by!**/generated/**internal/rpc/generated/accommodationv3_AccommodationProductInfoService_server.gois excluded by!**/generated/**internal/rpc/generated/accommodationv3_AccommodationProductListService_AccommodationProductList_client_method.gois excluded by!**/generated/**internal/rpc/generated/accommodationv3_AccommodationProductListService_AccommodationProductList_server_method.gois excluded by!**/generated/**internal/rpc/generated/accommodationv3_AccommodationProductListService_client.gois excluded by!**/generated/**internal/rpc/generated/accommodationv3_AccommodationProductListService_server.gois excluded by!**/generated/**internal/rpc/generated/accommodationv3_AccommodationSearchService_AccommodationSearch_client_method.gois excluded by!**/generated/**internal/rpc/generated/accommodationv3_AccommodationSearchService_AccommodationSearch_server_method.gois excluded by!**/generated/**internal/rpc/generated/accommodationv3_AccommodationSearchService_client.gois excluded by!**/generated/**internal/rpc/generated/accommodationv3_AccommodationSearchService_server.gois excluded by!**/generated/**internal/rpc/generated/activityv1_ActivityProductInfoService_ActivityProductInfo_server_method.gois excluded by!**/generated/**internal/rpc/generated/activityv1_ActivityProductInfoService_client.gois excluded by!**/generated/**internal/rpc/generated/activityv1_ActivityProductInfoService_server.gois excluded by!**/generated/**internal/rpc/generated/activityv1_ActivityProductListService_ActivityProductList_server_method.gois excluded by!**/generated/**internal/rpc/generated/activityv1_ActivityProductListService_client.gois excluded by!**/generated/**internal/rpc/generated/activityv1_ActivityProductListService_server.gois excluded by!**/generated/**internal/rpc/generated/activityv1_ActivitySearchService_ActivitySearch_server_method.gois excluded by!**/generated/**internal/rpc/generated/activityv1_ActivitySearchService_client.gois excluded by!**/generated/**internal/rpc/generated/activityv1_ActivitySearchService_server.gois excluded by!**/generated/**internal/rpc/generated/activityv2_ActivityProductInfoService_ActivityProductInfo_server_method.gois excluded by!**/generated/**internal/rpc/generated/activityv2_ActivityProductInfoService_client.gois excluded by!**/generated/**internal/rpc/generated/activityv2_ActivityProductInfoService_server.gois excluded by!**/generated/**internal/rpc/generated/activityv2_ActivityProductListService_ActivityProductList_server_method.gois excluded by!**/generated/**internal/rpc/generated/activityv2_ActivityProductListService_client.gois excluded by!**/generated/**internal/rpc/generated/activityv2_ActivityProductListService_server.gois excluded by!**/generated/**internal/rpc/generated/activityv2_ActivitySearchService_ActivitySearch_server_method.gois excluded by!**/generated/**internal/rpc/generated/activityv2_ActivitySearchService_client.gois excluded by!**/generated/**internal/rpc/generated/activityv2_ActivitySearchService_server.gois excluded by!**/generated/**internal/rpc/generated/activityv3_ActivityProductInfoService_ActivityProductInfo_client_method.gois excluded by!**/generated/**internal/rpc/generated/activityv3_ActivityProductInfoService_ActivityProductInfo_server_method.gois excluded by!**/generated/**internal/rpc/generated/activityv3_ActivityProductInfoService_client.gois excluded by!**/generated/**internal/rpc/generated/activityv3_ActivityProductInfoService_server.gois excluded by!**/generated/**internal/rpc/generated/activityv3_ActivityProductListService_ActivityProductList_client_method.gois excluded by!**/generated/**internal/rpc/generated/activityv3_ActivityProductListService_ActivityProductList_server_method.gois excluded by!**/generated/**internal/rpc/generated/activityv3_ActivityProductListService_client.gois excluded by!**/generated/**internal/rpc/generated/activityv3_ActivityProductListService_server.gois excluded by!**/generated/**internal/rpc/generated/activityv3_ActivitySearchService_ActivitySearch_client_method.gois excluded by!**/generated/**internal/rpc/generated/activityv3_ActivitySearchService_ActivitySearch_server_method.gois excluded by!**/generated/**internal/rpc/generated/activityv3_ActivitySearchService_client.gois excluded by!**/generated/**internal/rpc/generated/activityv3_ActivitySearchService_server.gois excluded by!**/generated/**internal/rpc/generated/bookv1_MintService_Mint_server_method.gois excluded by!**/generated/**internal/rpc/generated/bookv1_MintService_client.gois excluded by!**/generated/**internal/rpc/generated/bookv1_MintService_server.gois excluded by!**/generated/**internal/rpc/generated/bookv1_ValidationService_Validation_server_method.gois excluded by!**/generated/**internal/rpc/generated/bookv1_ValidationService_client.gois excluded by!**/generated/**internal/rpc/generated/bookv1_ValidationService_server.gois excluded by!**/generated/**internal/rpc/generated/bookv2_MintService_Mint_server_method.gois excluded by!**/generated/**internal/rpc/generated/bookv2_MintService_client.gois excluded by!**/generated/**internal/rpc/generated/bookv2_MintService_server.gois excluded by!**/generated/**internal/rpc/generated/bookv2_ValidationService_Validation_server_method.gois excluded by!**/generated/**internal/rpc/generated/bookv2_ValidationService_client.gois excluded by!**/generated/**internal/rpc/generated/bookv2_ValidationService_server.gois excluded by!**/generated/**internal/rpc/generated/bookv3_MintService_Mint_client_method.gois excluded by!**/generated/**internal/rpc/generated/bookv3_MintService_Mint_server_method.gois excluded by!**/generated/**internal/rpc/generated/bookv3_MintService_client.gois excluded by!**/generated/**internal/rpc/generated/bookv3_MintService_server.gois excluded by!**/generated/**internal/rpc/generated/bookv3_ValidationService_Validation_client_method.gois excluded by!**/generated/**internal/rpc/generated/bookv3_ValidationService_Validation_server_method.gois excluded by!**/generated/**internal/rpc/generated/bookv3_ValidationService_client.gois excluded by!**/generated/**internal/rpc/generated/bookv3_ValidationService_server.gois excluded by!**/generated/**internal/rpc/generated/infov1_CountryEntryRequirementsService_CountryEntryRequirements_server_method.gois excluded by!**/generated/**internal/rpc/generated/infov1_CountryEntryRequirementsService_client.gois excluded by!**/generated/**internal/rpc/generated/infov1_CountryEntryRequirementsService_server.gois excluded by!**/generated/**internal/rpc/generated/infov2_CountryEntryRequirementsService_CountryEntryRequirements_server_method.gois excluded by!**/generated/**internal/rpc/generated/infov2_CountryEntryRequirementsService_client.gois excluded by!**/generated/**internal/rpc/generated/infov2_CountryEntryRequirementsService_server.gois excluded by!**/generated/**internal/rpc/generated/insurancev1_InsuranceProductInfoService_InsuranceProductInfo_server_method.gois excluded by!**/generated/**internal/rpc/generated/insurancev1_InsuranceProductInfoService_client.gois excluded by!**/generated/**internal/rpc/generated/insurancev1_InsuranceProductInfoService_server.gois excluded by!**/generated/**internal/rpc/generated/insurancev1_InsuranceProductListService_InsuranceProductList_server_method.gois excluded by!**/generated/**internal/rpc/generated/insurancev1_InsuranceProductListService_client.gois excluded by!**/generated/**internal/rpc/generated/insurancev1_InsuranceProductListService_server.gois excluded by!**/generated/**internal/rpc/generated/insurancev1_InsuranceSearchService_InsuranceSearch_server_method.gois excluded by!**/generated/**internal/rpc/generated/insurancev1_InsuranceSearchService_client.gois excluded by!**/generated/**internal/rpc/generated/insurancev1_InsuranceSearchService_server.gois excluded by!**/generated/**internal/rpc/generated/pingv1_PingService_Ping_server_method.gois excluded by!**/generated/**internal/rpc/generated/pingv1_PingService_client.gois excluded by!**/generated/**internal/rpc/generated/pingv1_PingService_server.gois excluded by!**/generated/**internal/rpc/generated/register_client_services.gois excluded by!**/generated/**internal/rpc/generated/register_server_services.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv1_SeatMapAvailabilityService_SeatMapAvailability_server_method.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv1_SeatMapAvailabilityService_client.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv1_SeatMapAvailabilityService_server.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv1_SeatMapService_SeatMap_server_method.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv1_SeatMapService_client.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv1_SeatMapService_server.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv2_SeatMapAvailabilityService_SeatMapAvailability_server_method.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv2_SeatMapAvailabilityService_client.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv2_SeatMapAvailabilityService_server.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv2_SeatMapService_SeatMap_server_method.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv2_SeatMapService_client.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv2_SeatMapService_server.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv3_SeatMapAvailabilityService_SeatMapAvailability_client_method.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv3_SeatMapAvailabilityService_SeatMapAvailability_server_method.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv3_SeatMapAvailabilityService_client.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv3_SeatMapAvailabilityService_server.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv3_SeatMapService_SeatMap_client_method.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv3_SeatMapService_SeatMap_server_method.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv3_SeatMapService_client.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv3_SeatMapService_server.gois excluded by!**/generated/**internal/rpc/generated/transportv1_TransportSearchService_TransportSearch_server_method.gois excluded by!**/generated/**internal/rpc/generated/transportv1_TransportSearchService_client.gois excluded by!**/generated/**internal/rpc/generated/transportv1_TransportSearchService_server.gois excluded by!**/generated/**internal/rpc/generated/transportv2_TransportSearchService_TransportSearch_server_method.gois excluded by!**/generated/**internal/rpc/generated/transportv2_TransportSearchService_client.gois excluded by!**/generated/**internal/rpc/generated/transportv2_TransportSearchService_server.gois excluded by!**/generated/**internal/rpc/generated/transportv3_TransportProductListService_TransportProductList_client_method.gois excluded by!**/generated/**internal/rpc/generated/transportv3_TransportProductListService_TransportProductList_server_method.gois excluded by!**/generated/**internal/rpc/generated/transportv3_TransportProductListService_client.gois excluded by!**/generated/**internal/rpc/generated/transportv3_TransportProductListService_server.gois excluded by!**/generated/**internal/rpc/generated/transportv3_TransportSearchService_TransportSearch_client_method.gois excluded by!**/generated/**internal/rpc/generated/transportv3_TransportSearchService_TransportSearch_server_method.gois excluded by!**/generated/**internal/rpc/generated/transportv3_TransportSearchService_client.gois excluded by!**/generated/**internal/rpc/generated/transportv3_TransportSearchService_server.gois excluded by!**/generated/**internal/rpc/generated/unmarshal.gois excluded by!**/generated/**tests/e2e/bot/generated/client.gois excluded by!**/generated/**
📒 Files selected for processing (13)
examples/rpc/partner-plugin/server.go(31 hunks)go.mod(2 hunks)internal/messaging/noop_response_handler.go(1 hunks)internal/messaging/processor.go(4 hunks)internal/messaging/processor_test.go(2 hunks)internal/messaging/response_handler.go(2 hunks)internal/rpc/rpc.go(1 hunks)internal/rpc/server/server.go(2 hunks)scripts/generate_grpc_service_handlers.sh(8 hunks)templates/client.go.tpl(0 hunks)templates/server.go.tpl(1 hunks)templates/server_local_on_chain_method.go.tpl(1 hunks)templates/server_p2p_method.go.tpl(1 hunks)
💤 Files with no reviewable changes (1)
- templates/client.go.tpl
🚧 Files skipped from review as they are similar to previous changes (10)
- internal/messaging/response_handler.go
- templates/server_p2p_method.go.tpl
- internal/rpc/rpc.go
- internal/rpc/server/server.go
- internal/messaging/processor_test.go
- internal/messaging/noop_response_handler.go
- templates/server.go.tpl
- templates/server_local_on_chain_method.go.tpl
- go.mod
- internal/messaging/processor.go
🧰 Additional context used
🧠 Learnings (1)
examples/rpc/partner-plugin/server.go (1)
Learnt from: VjeraTurk
PR: chain4travel/camino-messenger-bot#89
File: pp-mock/handlers/accommodation/v3/accommodation_search.go:148-160
Timestamp: 2025-01-22T09:22:31.686Z
Learning: The AccommodationSearch service in v3 intentionally reuses the AccommodationSearchResult type from v2 as per the protocol definition. This is not a version compatibility issue.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Unit Tests
🔇 Additional comments (30)
examples/rpc/partner-plugin/server.go (2)
15-42: LGTM! Service imports have been properly updated to v3.The import statements have been correctly updated to include the new v3 versions of the services while maintaining backward compatibility with v1 and v2 versions where needed.
1308-1350: LGTM! Server setup is well-implemented.The main function properly:
- Registers all v3 services
- Handles port configuration via environment variables
- Includes error handling
- Enables gRPC reflection for debugging
scripts/generate_grpc_service_handlers.sh (28)
8-9: Introduce distinct server method templates for P2P and local on-chain services.
The addition ofSERVER_P2P_METHOD_TEMPLATEandSERVER_LOCAL_ON_CHAIN_TEMPLATEsupports the new service tagging and handler generation for different routing types. Even if both variables currently point to the same directory, this separation enables future flexibility and clearer code‐organization.
11-12: Separate output path variables for generated services.
DefiningP2P_OUTPATHandLOCAL_OUTPATHas separate variables—despite having the same value now—sets the stage for easier modifications if distinct directories become necessary later on.
15-17: Updated registration and unmarshalling file paths.
The updated assignments ofREGISTER_SERVICES_SERVER_FILE,REGISTER_SERVICES_CLIENT_FILE, andUNMARSHALING_FILEnow use theP2P_OUTPATH. This change is consistent with the restructured generation logic for P2P services.
53-59: Ensure proper handling of EMPTY_IMPORT substitution.
Appending a sed replacement for{{EMPTY_IMPORT}}in the general parameters allows dynamic injection based on message type. The subsequent reset of the variable (line 59) appears intentional; please verify that its timing does not inadvertently override needed substitutions in later steps.
128-156: Local on-chain Server Generation Flow.
The branch for services withROUTING == "local"andON_CHAIN == "true"correctly uses theSERVER_LOCAL_ON_CHAIN_TEMPLATEand outputs files toLOCAL_OUTPATH. As with other command invocations throughout the script, consider surrounding variable expansions with quotes for enhanced safety.
159-182: Registering Server Services.
Thegenerate_register_services_serverfunction crafts a registration file neatly by iterating over provided services. Its straightforward structure aids in maintainability.
183-212: Registering Client Services.
Thegenerate_register_services_clientfunction generates client service registration code cleanly. Minor improvements, such as consistent quoting and indentation, could further enhance readability.
214-252: Unmarshalling Code Generation.
The functiongenerate_unmarshallingeffectively loops over import paths and unmarshal methods to generate an unmarshalling helper. The switch-case logic is clear; just ensure that the proto-based unmarshal logic remains in sync with upstream changes.
253-302: E2E Bot Client Generation.
The procedure for generating the end-to-end bot client (lines 253–302) is methodical and well-structured. Building client field constructors via array aggregation is a neat solution.
310-322: SDK Version Extraction and Dependency Download.
The extraction of gRPC and protocol buffers versions fromgo.modis correctly implemented, and the use ofgo getto fetch the corresponding SDK versions is appropriate. Verify that the regex patterns meet all versioning formats that might appear.
349-354: SDK GRPC and PB Paths Computation.
Constructing theSDK_GRPC_PATHandSDK_PB_PATHfromGO_PATHand the extracted version numbers is correctly handled. This relies on the standard Go module layout.
355-362: gofumpt Installation Verification.
The script installsgofumptand validates its presence before proceeding. Although checking for the file’s existence is effective, you might also consider verifying its exit status when run.
363-371: Initialization of Arrays for Service Registration and E2E Generation.
Declaring the arrays (e.g.,SERVICES_TO_REGISTER,PROTO_INCLUDES_FOR_UNMARSHALLING) clearly sets up data structures for the generation process. Their usage in later parts of the script is straightforward.
372-388: File Filtering and Blacklist Check.
The loop correctly applies an optional filter and skips files matching any entries in theDEFAULT_BLACKLIST. Usingcontinue 2to exit the current file iteration is an effective approach.
402-412: Extracting FQPN and Corresponding PB File.
The script derives the fully qualified protocol name (FQPN) and locates the associated generated PB file using grep patterns. Ensure these regex patterns remain valid if the proto file format evolves.
414-417: PB File Existence Validation.
The explicit check for the existence of the related PB file helps avoid later errors. Exiting on failure is a good safety net.
419-421: Extract common types version with tail filtering.
Usingtail -n 1to select the appropriate match forCOMMON_TYPES_VERSIONeffectively mitigates the duplicate output issue noted in previous revisions.
422-426: Verbose Logging of Service Details.
Echoing critical service information (FQPN, service name, extracted tags, etc.) provides useful feedback during generation for debugging purposes.
458-461: Populating Unmarshal Methods for P2P Routing.
Appending corresponding unmarshal method definitions for P2P services ensures that both request and response types are processed later, which meets the generation requirements.
463-464: Logging Extracted Method Information.
The echo statement confirms the extracted method along with its determined input and output types, which aids in verifying the parsing logic.
466-470: Configuring Proto and GRPC Include Paths.
Calculating the suffix fromFQPNand constructingPROTO_INCLUDEandGRPC_INCLUDEpaths is performed as expected. This assumes the module paths follow the standard directory structure.
471-477: Invoking the Template Generation Function.
The call togenerate_with_templatespasses all relevant data (FQPN, service details, method arrays, include paths, version, etc.) necessary for generating the gRPC service handlers. Double-check that the argument order and expected types match the function’s definition.
478-482: Aggregating E2E Generation Data.
Appending GRPC includes, package names, and client field information for the end-to-end bot client is correctly handled, ensuring that later steps have the required context for testing.
483-486: Conditional Service Registration for P2P Routing.
Only for P2P routing, the script collects service names and proto includes for later registration, which cleanly separates the registration logic from local on-chain services.
488-488: Log Output Formatting.
An empty echo is used to improve log readability and separate output sections.
489-489: End of file processing loop.
The while loop ends after processing all files from the SDK directory, marking the conclusion of the discovery phase.
491-494: Generate Registration, Unmarshalling, and E2E Client Files.
The successive calls to generate the registration for server and client services, the unmarshal helper, and the E2E bot client complete the automated code generation workflow.
496-499: Apply gofumpt Formatting.
The final steps rungofumpton all generated directories to ensure consistent Go code formatting. Verify that$FUMPTcorrectly points to the installed formatter binary.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
scripts/generate_grpc_service_handlers.sh (1)
103-127:⚠️ Potential issueP2P server methods generation – Incorrect template substitution variable
Within the server methods generation loop, particularly at line 126, the substitution uses:- sed -i -e "s#{{TEMPLATE}}#$SERVER_METHOD_TEMPLATE#g" "$METHOD_GEN_FILE" + sed -i -e "s#{{TEMPLATE}}#$SERVER_P2P_METHOD_TEMPLATE#g" "$METHOD_GEN_FILE"Since
$SERVER_METHOD_TEMPLATEis not defined (the new variable is$SERVER_P2P_METHOD_TEMPLATE), this will lead to incorrect substitutions.
🧹 Nitpick comments (5)
scripts/generate_grpc_service_handlers.sh (5)
11-12: Output paths for service handlers
BothP2P_OUTPATHandLOCAL_OUTPATHare currently set to the same directory. This matches the current design per PR discussion, but if future segregation is desired, consider differentiating these paths.
70-82: Client method generation loop & handling of Empty responses
Within the loop generating client methods, whenOUTPUTequals"Empty", the script sets
EMPTY_IMPORT='"google.golang.org/protobuf/types/known/emptypb"'
and then assignsTYPE_PACKAGE="emptypb".
Please verify that reassigningTYPE_PACKAGEdoes not adversely affect subsequent iterations where the original package name is expected; consider caching the original value if different methods require distinct types.
128-156: Local on-chain server methods generation
The branch handling local on-chain services correctly uses theSERVER_LOCAL_ON_CHAIN_TEMPLATEfor generating server methods. A couple of minor notes:
- The sed commands in lines 132, 152, and 153 use unquoted variables (e.g.
$GENERAL_PARAM_REPLACE), which might lead to word-splitting issues; consider using the array form (as done earlier) for robustness.- Overall, the logic and separation of concerns in this block are well implemented.
372-384: File scanning with filter and blacklist logic
The loop that reads files for processing implements filtering via the optional filter and checks against a blacklist effectively. One minor note: ensure that the pattern matching on blacklisted words (using$file =~ $blacklisted) is robust for all expected inputs.
389-401: Extraction of annotation tags from files
The script extracts custom annotation tags (type:,routing:, andon-chain:) using shell parameter expansion. Consider adding error checking if any of these tags are missing to avoid potential misparsing downstream.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/generate_grpc_service_handlers.sh(11 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Unit Tests
- GitHub Check: Static Analysis
- GitHub Check: E2E Tests
- GitHub Check: Build docker image
🔇 Additional comments (24)
scripts/generate_grpc_service_handlers.sh (24)
8-9: Introduce distinct server method templates
The additions ofSERVER_P2P_METHOD_TEMPLATEandSERVER_LOCAL_ON_CHAIN_TEMPLATEclearly delineate the templates for P2P versus local on‐chain service methods. This improves readability and future extensibility.
15-17: Define service registration and unmarshalling file paths
The definitions forREGISTER_SERVICES_SERVER_FILE,REGISTER_SERVICES_CLIENT_FILE, andUNMARSHALING_FILEuse the output path appropriately. Ensure that if local and P2P handlers ever reside in separate directories, these assignments are updated accordingly.
21-21: DEFAULT_BLACKLIST update
The updated blacklist string is clear and well documented. If more filters are required later, consider making this configurable.
53-53: Include EMPTY_IMPORT substitution in parameter replacements
Adding the substitution for{{EMPTY_IMPORT}}intoGENERAL_PARAM_REPLACEimproves the template’s flexibility.
59-60: Reset EMPTY_IMPORT before generation
ResettingEMPTY_IMPORTjust before entering the routing-based code generation ensures that stale values aren’t carried over.
61-69: Client file generation using templates
The block that generates the client file (lines 61–69) uses the provided template and performs the sed substitutions correctly. The usage of"${replace_params[@]}"is appropriate.
95-101: P2P server file generation
The generation of the P2P server file (lines 95–101) properly applies the template and sed substitutions. The code structure is clear and follows the approach used for client generation.
159-181: Register server services generation function
Thegenerate_register_services_serverfunction cleanly creates the registration file. The use of a heredoc-style block for generating the content is clear.
183-212: Register client services generation function
This function constructs the client registration file and maps service names to service instances appropriately. The logic is straightforward and consistent with the overall design.
214-251: Unmarshalling generation function
Thegenerate_unmarshallingfunction efficiently builds an unmarshalling helper with necessary imports and a switch-case for different message types. The approach is clear and robust.
253-301: E2E bot client generation
The code generating the E2E client neatly constructs client structures with dynamic fields and constructors. The concatenation of field names and package references is handled well.
303-309: Output directory cleanup and creation
The commands that remove and then create both$P2P_OUTPATHand$LOCAL_OUTPATHare straightforward and ensure a clean state for generation.
324-342: GOPATH determination with fallbacks
The script’s logic for determiningGO_PATH(using$GOPATH,go env, or default locations) is robust and enhances portability across environments.
355-362: Ensuring gofumpt is installed
The check for gofumpt’s existence and proper installation improves code consistency by enforcing formatting standards.
364-371: Array declarations for service generation
The declaration of arrays for services, includes, and related fields is clear and sets up the subsequent file scanning properly.
409-417: PB file derivation and validation
The logic to determine the corresponding protocol buffers file and validate its existence is robust and provides a clear error message on failure.
419-419: Extract COMMON_TYPES_VERSION robustly
Usingtail -n 1to extract theCOMMON_TYPES_VERSIONfrom the pb file addresses the issue of multiple results and ensures a single correct version is captured.
423-424: Informative tag logging
The echoed output displaying the service tags and related metadata aids debugging and confirms that tag extraction is functioning as intended.
427-444: Parsing method parameters for service methods
The block that reads each method’s parameters (via grep, cut, and shell expansion) is functional. It may be beneficial to verify that this parsing holds across any variations in method signatures.
458-461: Conditional unmarshalling methods addition for P2P routing
Appending additional unmarshalling methods whenROUTINGequals"p2p"is correctly guarded by the condition and logically sound.
472-477: Construct protocol and gRPC include paths and trigger template generation
The derivation ofSUFFIX, followed by the construction of bothPROTO_INCLUDEandGRPC_INCLUDE, is well integrated before callinggenerate_with_templates().
478-486: Update arrays for E2E generation and services registration
Appending the computed include paths and service names into the respective arrays is performed conditionally for p2p routing, aligning with the intended logic.
491-494: Final generation of registration, unmarshalling, and E2E client files
The sequential calls togenerate_register_services_server,generate_register_services_client,generate_unmarshalling, andgenerate_e2e_bot_clientensure complete output file generation.
496-500: Format generated code using gofumpt
Running gofumpt on all generated directories guarantees consistent formatting. This final formatting step reinforces code quality.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/messaging/response_handler.go (2)
49-50: Enhance the method documentation.The documentation for
ProcessRequestMessageshould be more descriptive. Consider adding details about:
- The purpose of request processing
- Expected behavior for different request types
- Error conditions and handling
- // Processes incoming request + // ProcessRequestMessage handles incoming request messages, performing any necessary + // validations and business logic. It supports different request types (e.g., cancellations) + // and returns an error if the request cannot be processed.
127-129: Remove unused comment.The comment about cancellation messages appears to be unused and can be removed. If it's meant to serve as documentation, consider moving it to the method's documentation comment.
func (h *evmResponseHandler) ProcessRequestMessage( _ context.Context, _ *types.Message, _ protoreflect.ProtoMessage, ) error { // TODO: @VjeraTurk A the logic for processing these request messages - /* - Cancellation messages will serve as an example of this type - */ return nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/messaging/response_handler.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Unit Tests
- GitHub Check: Static Analysis
🔇 Additional comments (1)
internal/messaging/response_handler.go (1)
121-131: Enhance the placeholder implementation.The current implementation silently ignores all requests by returning nil. Consider:
- Adding a "not implemented" error until the actual logic is added
- Including type switches for different request types (similar to
ProcessResponseMessage)- Adding logging for debugging purposes
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
scripts/generate_grpc_service_handlers.sh (3)
128-155: Local On-Chain Service Generation Block
In theelifbranch for handling local on-chain services ([[ "$ROUTING" == "local" && "$ON_CHAIN" == "true" ]]), you correctly useSERVER_TEMPLATEfor the main server file andSERVER_LOCAL_ON_CHAIN_TEMPLATEfor server methods. However, note that the sed invocations on lines 132, 152, 153, and 154 use unquoted variables (e.g.$GENERAL_PARAM_REPLACEand$METHOD_PARAM_REPLACE). Quoting them (e.g."${GENERAL_PARAM_REPLACE}") would improve resilience against word-splitting or whitespace issues.[helpful]
389-397: Service Tag Extraction Implementation
The code block extracting service tags (lines 389–397) uses shell parameter expansion to deriveSERVICE_TYPE,ROUTING, andON_CHAINfrom the annotation line. While this works under the assumption of a well-formed annotation, consider using more robust parsing (for example, leveraginggrep -oPwith proper regex and default fallbacks) to handle cases when tags might be missing or formatted unexpectedly.
497-499: Gofumpt Command Invocation
The script runs thegofumptformatter on the generated directories. In the commands on lines 497–499, consider quoting the directory variables (for example,"$P2P_OUTPATH") to avoid potential issues if any of these paths contain spaces.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (37)
internal/rpc/generated/accommodationv1_AccommodationProductInfoService_AccommodationProductInfo_server_method.gois excluded by!**/generated/**internal/rpc/generated/accommodationv1_AccommodationProductListService_AccommodationProductList_server_method.gois excluded by!**/generated/**internal/rpc/generated/accommodationv1_AccommodationSearchService_AccommodationSearch_server_method.gois excluded by!**/generated/**internal/rpc/generated/accommodationv2_AccommodationProductInfoService_AccommodationProductInfo_server_method.gois excluded by!**/generated/**internal/rpc/generated/accommodationv2_AccommodationProductListService_AccommodationProductList_server_method.gois excluded by!**/generated/**internal/rpc/generated/accommodationv2_AccommodationSearchService_AccommodationSearch_server_method.gois excluded by!**/generated/**internal/rpc/generated/accommodationv3_AccommodationProductInfoService_AccommodationProductInfo_server_method.gois excluded by!**/generated/**internal/rpc/generated/accommodationv3_AccommodationProductListService_AccommodationProductList_server_method.gois excluded by!**/generated/**internal/rpc/generated/accommodationv3_AccommodationSearchService_AccommodationSearch_server_method.gois excluded by!**/generated/**internal/rpc/generated/activityv1_ActivityProductInfoService_ActivityProductInfo_server_method.gois excluded by!**/generated/**internal/rpc/generated/activityv1_ActivityProductListService_ActivityProductList_server_method.gois excluded by!**/generated/**internal/rpc/generated/activityv1_ActivitySearchService_ActivitySearch_server_method.gois excluded by!**/generated/**internal/rpc/generated/activityv2_ActivityProductInfoService_ActivityProductInfo_server_method.gois excluded by!**/generated/**internal/rpc/generated/activityv2_ActivityProductListService_ActivityProductList_server_method.gois excluded by!**/generated/**internal/rpc/generated/activityv2_ActivitySearchService_ActivitySearch_server_method.gois excluded by!**/generated/**internal/rpc/generated/activityv3_ActivityProductInfoService_ActivityProductInfo_server_method.gois excluded by!**/generated/**internal/rpc/generated/activityv3_ActivityProductListService_ActivityProductList_server_method.gois excluded by!**/generated/**internal/rpc/generated/activityv3_ActivitySearchService_ActivitySearch_server_method.gois excluded by!**/generated/**internal/rpc/generated/bookv1_MintService_Mint_server_method.gois excluded by!**/generated/**internal/rpc/generated/bookv1_ValidationService_Validation_server_method.gois excluded by!**/generated/**internal/rpc/generated/bookv2_MintService_Mint_server_method.gois excluded by!**/generated/**internal/rpc/generated/bookv2_ValidationService_Validation_server_method.gois excluded by!**/generated/**internal/rpc/generated/bookv3_MintService_Mint_server_method.gois excluded by!**/generated/**internal/rpc/generated/bookv3_ValidationService_Validation_server_method.gois excluded by!**/generated/**internal/rpc/generated/infov1_CountryEntryRequirementsService_CountryEntryRequirements_server_method.gois excluded by!**/generated/**internal/rpc/generated/infov2_CountryEntryRequirementsService_CountryEntryRequirements_server_method.gois excluded by!**/generated/**internal/rpc/generated/pingv1_PingService_Ping_server_method.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv1_SeatMapAvailabilityService_SeatMapAvailability_server_method.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv1_SeatMapService_SeatMap_server_method.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv2_SeatMapAvailabilityService_SeatMapAvailability_server_method.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv2_SeatMapService_SeatMap_server_method.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv3_SeatMapAvailabilityService_SeatMapAvailability_server_method.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv3_SeatMapService_SeatMap_server_method.gois excluded by!**/generated/**internal/rpc/generated/transportv1_TransportSearchService_TransportSearch_server_method.gois excluded by!**/generated/**internal/rpc/generated/transportv2_TransportSearchService_TransportSearch_server_method.gois excluded by!**/generated/**internal/rpc/generated/transportv3_TransportProductListService_TransportProductList_server_method.gois excluded by!**/generated/**internal/rpc/generated/transportv3_TransportSearchService_TransportSearch_server_method.gois excluded by!**/generated/**
📒 Files selected for processing (1)
scripts/generate_grpc_service_handlers.sh(11 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Unit Tests
- GitHub Check: Static Analysis
- GitHub Check: Build docker image
🔇 Additional comments (6)
scripts/generate_grpc_service_handlers.sh (6)
8-9: Added Server Template Variables
You've introduced two new variables,SERVER_P2P_METHOD_TEMPLATEandSERVER_LOCAL_ON_CHAIN_TEMPLATE, to distinguish between P2P and local on-chain server method templates. This change clearly supports the new service tags and routing logic.
11-12: Defined Output Paths for Service Generation
BothP2P_OUTPATHandLOCAL_OUTPATHare set to"internal/rpc/generated". This is consistent with the discussion notes; however, if in the future you decide to truly separate generated files into distinct directories based on the service tag, you may need to update these paths accordingly.
15-17: Registration Files Variables Alignment
The registration file variables (REGISTER_SERVICES_SERVER_FILE,REGISTER_SERVICES_CLIENT_FILE, andUNMARSHALING_FILE) have been updated to point to the P2P output directory. Please verify that for local on-chain services no separate registration file is required—or that the current design (registering only P2P services) is intentional.
53-53: Addition of EMPTY_IMPORT Substitution
Appending the substitution for the{{EMPTY_IMPORT}}placeholder toGENERAL_PARAM_REPLACEensures that templates containing this marker are handled gracefully. Make sure that the corresponding placeholder is indeed used in your templates.
59-59: Reset of EMPTY_IMPORT Variable
ResettingEMPTY_IMPORTto an empty string immediately after the sed replacement parameters are set avoids any unintended leakage of a previously set value to subsequent operations. Just confirm that sections later (e.g. in client method generation) correctly setEMPTY_IMPORTwhen needed.
419-419: Robust Extraction of COMMON_TYPES_VERSION
The extraction ofCOMMON_TYPES_VERSIONusinggreppiped totail -n 1is a smart fix to ensure that, in case of multiple matches, only the last occurrence (presumably the correct one) is selected. This aligns with the patch discussed previously.
7153f71 to
d37717c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
examples/rpc/partner-plugin/server.go (1)
138-140:⚠️ Potential issueFix the year typo in the date.
The year is set to
20024which appears to be a typo.- StartDatetime: timestamppb.New(time.Date(20024, 9, 20, 11, 0o0, 0, 0, time.UTC)), // summary.start - EndDatetime: timestamppb.New(time.Date(20024, 9, 20, 12, 0o0, 0, 0, time.UTC)), + StartDatetime: timestamppb.New(time.Date(2024, 9, 20, 11, 0o0, 0, 0, time.UTC)), // summary.start + EndDatetime: timestamppb.New(time.Date(2024, 9, 20, 12, 0o0, 0, 0, time.UTC)),
♻️ Duplicate comments (1)
scripts/generate_grpc_service_handlers.sh (1)
389-401: 🛠️ Refactor suggestionImprove service tag extraction robustness.
The current tag extraction logic assumes a specific format and order. It should be more robust to handle variations in tag format and missing tags.
-# Extract the annotation line -TAGS_LINE=$(grep -E '@custom:cmp-service' "$file") - -# Use shell parameter expansion to extract values -SERVICE_TYPE=${TAGS_LINE#*type:} -SERVICE_TYPE=${SERVICE_TYPE%% *} - -ROUTING=${TAGS_LINE#*routing:} -ROUTING=${ROUTING%% *} - -ON_CHAIN=${TAGS_LINE#*on-chain:} -ON_CHAIN=${ON_CHAIN%%[[:space:]]*} +# Extract tags using grep with specific patterns +SERVICE_TYPE=$(grep -E '@custom:cmp-service.*type:' "$file" | grep -oP 'type:\K[^ ]+' || echo "unknown") +ROUTING=$(grep -E '@custom:cmp-service.*routing:' "$file" | grep -oP 'routing:\K[^ ]+' || echo "p2p") +ON_CHAIN=$(grep -E '@custom:cmp-service.*on-chain:' "$file" | grep -oP 'on-chain:\K[^ ]+' || echo "false") + +# Validate extracted values +if [[ "$SERVICE_TYPE" == "unknown" ]]; then + echo "⚠️ Warning: Missing service type tag in $file" +fi
🧹 Nitpick comments (3)
internal/rpc/server/server.go (1)
129-147: Consider extracting common error handling logic.The error handling and metadata processing logic is duplicated between
HandleP2PMessageRequestandHandleMessageRequest. Consider extracting this into a helper method to improve maintainability.+func (s *server) processRequestWithMetadata(ctx context.Context, spanName string, processor func(context.Context, *types.Message) (*types.Message, error), msg *types.Message) (protoreflect.ProtoMessage, error) { + ctx, span := s.tracer.Start(ctx, spanName, trace.WithSpanKind(trace.SpanKindServer)) + defer span.End() + md, err := s.processMetadata(ctx, s.tracer.TraceIDForSpan(span)) + if err != nil { + return nil, fmt.Errorf("error processing metadata: %w", err) + } + msg.Metadata = md + response, err := processor(ctx, msg) + if err != nil { + return nil, fmt.Errorf("error processing request: %w", err) + } + response.Metadata.Stamp(fmt.Sprintf("%s-%s", s.Checkpoint(), "processed")) + err = grpc.SendHeader(ctx, response.Metadata.ToGrpcMD()) + return response.Content, err +}internal/messaging/processor.go (1)
274-300: Consider adding error handling for nil messages.The local message processing methods should validate that the input message is not nil before processing.
func (p *messageProcessor) ProcessIncomingLocalMessage( ctx context.Context, msg *types.Message, ) (*types.Message, error) { + if msg == nil { + return nil, fmt.Errorf("message cannot be nil") + } _, responseMsg, err := p.ProcessIncomingLocalMessageAndGetResponse(ctx, msg) if err != nil { return responseMsg, err } return responseMsg, nil }examples/rpc/partner-plugin/server.go (1)
1020-1021: Replace hardcoded image URLs with configuration values.The code contains hardcoded image URLs pointing to
camino.network. These should be moved to a configuration file or environment variables for better maintainability.- Url: "https://camino.network/static/images/6HibYS9gzR-1800.webp", + Url: config.GetImageBaseURL() + "/6HibYS9gzR-1800.webp",Also applies to: 1075-1076
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (137)
go.sumis excluded by!**/*.suminternal/rpc/generated/accommodationv1_AccommodationProductInfoService_AccommodationProductInfo_server_method.gois excluded by!**/generated/**internal/rpc/generated/accommodationv1_AccommodationProductInfoService_client.gois excluded by!**/generated/**internal/rpc/generated/accommodationv1_AccommodationProductInfoService_server.gois excluded by!**/generated/**internal/rpc/generated/accommodationv1_AccommodationProductListService_AccommodationProductList_server_method.gois excluded by!**/generated/**internal/rpc/generated/accommodationv1_AccommodationProductListService_client.gois excluded by!**/generated/**internal/rpc/generated/accommodationv1_AccommodationProductListService_server.gois excluded by!**/generated/**internal/rpc/generated/accommodationv1_AccommodationSearchService_AccommodationSearch_server_method.gois excluded by!**/generated/**internal/rpc/generated/accommodationv1_AccommodationSearchService_client.gois excluded by!**/generated/**internal/rpc/generated/accommodationv1_AccommodationSearchService_server.gois excluded by!**/generated/**internal/rpc/generated/accommodationv2_AccommodationProductInfoService_AccommodationProductInfo_server_method.gois excluded by!**/generated/**internal/rpc/generated/accommodationv2_AccommodationProductInfoService_client.gois excluded by!**/generated/**internal/rpc/generated/accommodationv2_AccommodationProductInfoService_server.gois excluded by!**/generated/**internal/rpc/generated/accommodationv2_AccommodationProductListService_AccommodationProductList_server_method.gois excluded by!**/generated/**internal/rpc/generated/accommodationv2_AccommodationProductListService_client.gois excluded by!**/generated/**internal/rpc/generated/accommodationv2_AccommodationProductListService_server.gois excluded by!**/generated/**internal/rpc/generated/accommodationv2_AccommodationSearchService_AccommodationSearch_server_method.gois excluded by!**/generated/**internal/rpc/generated/accommodationv2_AccommodationSearchService_client.gois excluded by!**/generated/**internal/rpc/generated/accommodationv2_AccommodationSearchService_server.gois excluded by!**/generated/**internal/rpc/generated/accommodationv3_AccommodationProductInfoService_AccommodationProductInfo_client_method.gois excluded by!**/generated/**internal/rpc/generated/accommodationv3_AccommodationProductInfoService_AccommodationProductInfo_server_method.gois excluded by!**/generated/**internal/rpc/generated/accommodationv3_AccommodationProductInfoService_client.gois excluded by!**/generated/**internal/rpc/generated/accommodationv3_AccommodationProductInfoService_server.gois excluded by!**/generated/**internal/rpc/generated/accommodationv3_AccommodationProductListService_AccommodationProductList_client_method.gois excluded by!**/generated/**internal/rpc/generated/accommodationv3_AccommodationProductListService_AccommodationProductList_server_method.gois excluded by!**/generated/**internal/rpc/generated/accommodationv3_AccommodationProductListService_client.gois excluded by!**/generated/**internal/rpc/generated/accommodationv3_AccommodationProductListService_server.gois excluded by!**/generated/**internal/rpc/generated/accommodationv3_AccommodationSearchService_AccommodationSearch_client_method.gois excluded by!**/generated/**internal/rpc/generated/accommodationv3_AccommodationSearchService_AccommodationSearch_server_method.gois excluded by!**/generated/**internal/rpc/generated/accommodationv3_AccommodationSearchService_client.gois excluded by!**/generated/**internal/rpc/generated/accommodationv3_AccommodationSearchService_server.gois excluded by!**/generated/**internal/rpc/generated/activityv1_ActivityProductInfoService_ActivityProductInfo_server_method.gois excluded by!**/generated/**internal/rpc/generated/activityv1_ActivityProductInfoService_client.gois excluded by!**/generated/**internal/rpc/generated/activityv1_ActivityProductInfoService_server.gois excluded by!**/generated/**internal/rpc/generated/activityv1_ActivityProductListService_ActivityProductList_server_method.gois excluded by!**/generated/**internal/rpc/generated/activityv1_ActivityProductListService_client.gois excluded by!**/generated/**internal/rpc/generated/activityv1_ActivityProductListService_server.gois excluded by!**/generated/**internal/rpc/generated/activityv1_ActivitySearchService_ActivitySearch_server_method.gois excluded by!**/generated/**internal/rpc/generated/activityv1_ActivitySearchService_client.gois excluded by!**/generated/**internal/rpc/generated/activityv1_ActivitySearchService_server.gois excluded by!**/generated/**internal/rpc/generated/activityv2_ActivityProductInfoService_ActivityProductInfo_server_method.gois excluded by!**/generated/**internal/rpc/generated/activityv2_ActivityProductInfoService_client.gois excluded by!**/generated/**internal/rpc/generated/activityv2_ActivityProductInfoService_server.gois excluded by!**/generated/**internal/rpc/generated/activityv2_ActivityProductListService_ActivityProductList_server_method.gois excluded by!**/generated/**internal/rpc/generated/activityv2_ActivityProductListService_client.gois excluded by!**/generated/**internal/rpc/generated/activityv2_ActivityProductListService_server.gois excluded by!**/generated/**internal/rpc/generated/activityv2_ActivitySearchService_ActivitySearch_server_method.gois excluded by!**/generated/**internal/rpc/generated/activityv2_ActivitySearchService_client.gois excluded by!**/generated/**internal/rpc/generated/activityv2_ActivitySearchService_server.gois excluded by!**/generated/**internal/rpc/generated/activityv3_ActivityProductInfoService_ActivityProductInfo_client_method.gois excluded by!**/generated/**internal/rpc/generated/activityv3_ActivityProductInfoService_ActivityProductInfo_server_method.gois excluded by!**/generated/**internal/rpc/generated/activityv3_ActivityProductInfoService_client.gois excluded by!**/generated/**internal/rpc/generated/activityv3_ActivityProductInfoService_server.gois excluded by!**/generated/**internal/rpc/generated/activityv3_ActivityProductListService_ActivityProductList_client_method.gois excluded by!**/generated/**internal/rpc/generated/activityv3_ActivityProductListService_ActivityProductList_server_method.gois excluded by!**/generated/**internal/rpc/generated/activityv3_ActivityProductListService_client.gois excluded by!**/generated/**internal/rpc/generated/activityv3_ActivityProductListService_server.gois excluded by!**/generated/**internal/rpc/generated/activityv3_ActivitySearchService_ActivitySearch_client_method.gois excluded by!**/generated/**internal/rpc/generated/activityv3_ActivitySearchService_ActivitySearch_server_method.gois excluded by!**/generated/**internal/rpc/generated/activityv3_ActivitySearchService_client.gois excluded by!**/generated/**internal/rpc/generated/activityv3_ActivitySearchService_server.gois excluded by!**/generated/**internal/rpc/generated/bookv1_MintService_Mint_server_method.gois excluded by!**/generated/**internal/rpc/generated/bookv1_MintService_client.gois excluded by!**/generated/**internal/rpc/generated/bookv1_MintService_server.gois excluded by!**/generated/**internal/rpc/generated/bookv1_ValidationService_Validation_server_method.gois excluded by!**/generated/**internal/rpc/generated/bookv1_ValidationService_client.gois excluded by!**/generated/**internal/rpc/generated/bookv1_ValidationService_server.gois excluded by!**/generated/**internal/rpc/generated/bookv2_MintService_Mint_server_method.gois excluded by!**/generated/**internal/rpc/generated/bookv2_MintService_client.gois excluded by!**/generated/**internal/rpc/generated/bookv2_MintService_server.gois excluded by!**/generated/**internal/rpc/generated/bookv2_ValidationService_Validation_server_method.gois excluded by!**/generated/**internal/rpc/generated/bookv2_ValidationService_client.gois excluded by!**/generated/**internal/rpc/generated/bookv2_ValidationService_server.gois excluded by!**/generated/**internal/rpc/generated/bookv3_MintService_Mint_client_method.gois excluded by!**/generated/**internal/rpc/generated/bookv3_MintService_Mint_server_method.gois excluded by!**/generated/**internal/rpc/generated/bookv3_MintService_client.gois excluded by!**/generated/**internal/rpc/generated/bookv3_MintService_server.gois excluded by!**/generated/**internal/rpc/generated/bookv3_ValidationService_Validation_client_method.gois excluded by!**/generated/**internal/rpc/generated/bookv3_ValidationService_Validation_server_method.gois excluded by!**/generated/**internal/rpc/generated/bookv3_ValidationService_client.gois excluded by!**/generated/**internal/rpc/generated/bookv3_ValidationService_server.gois excluded by!**/generated/**internal/rpc/generated/infov1_CountryEntryRequirementsService_CountryEntryRequirements_server_method.gois excluded by!**/generated/**internal/rpc/generated/infov1_CountryEntryRequirementsService_client.gois excluded by!**/generated/**internal/rpc/generated/infov1_CountryEntryRequirementsService_server.gois excluded by!**/generated/**internal/rpc/generated/infov2_CountryEntryRequirementsService_CountryEntryRequirements_server_method.gois excluded by!**/generated/**internal/rpc/generated/infov2_CountryEntryRequirementsService_client.gois excluded by!**/generated/**internal/rpc/generated/infov2_CountryEntryRequirementsService_server.gois excluded by!**/generated/**internal/rpc/generated/insurancev1_InsuranceProductInfoService_InsuranceProductInfo_server_method.gois excluded by!**/generated/**internal/rpc/generated/insurancev1_InsuranceProductInfoService_client.gois excluded by!**/generated/**internal/rpc/generated/insurancev1_InsuranceProductInfoService_server.gois excluded by!**/generated/**internal/rpc/generated/insurancev1_InsuranceProductListService_InsuranceProductList_server_method.gois excluded by!**/generated/**internal/rpc/generated/insurancev1_InsuranceProductListService_client.gois excluded by!**/generated/**internal/rpc/generated/insurancev1_InsuranceProductListService_server.gois excluded by!**/generated/**internal/rpc/generated/insurancev1_InsuranceSearchService_InsuranceSearch_server_method.gois excluded by!**/generated/**internal/rpc/generated/insurancev1_InsuranceSearchService_client.gois excluded by!**/generated/**internal/rpc/generated/insurancev1_InsuranceSearchService_server.gois excluded by!**/generated/**internal/rpc/generated/pingv1_PingService_Ping_server_method.gois excluded by!**/generated/**internal/rpc/generated/pingv1_PingService_client.gois excluded by!**/generated/**internal/rpc/generated/pingv1_PingService_server.gois excluded by!**/generated/**internal/rpc/generated/register_client_services.gois excluded by!**/generated/**internal/rpc/generated/register_server_services.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv1_SeatMapAvailabilityService_SeatMapAvailability_server_method.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv1_SeatMapAvailabilityService_client.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv1_SeatMapAvailabilityService_server.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv1_SeatMapService_SeatMap_server_method.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv1_SeatMapService_client.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv1_SeatMapService_server.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv2_SeatMapAvailabilityService_SeatMapAvailability_server_method.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv2_SeatMapAvailabilityService_client.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv2_SeatMapAvailabilityService_server.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv2_SeatMapService_SeatMap_server_method.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv2_SeatMapService_client.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv2_SeatMapService_server.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv3_SeatMapAvailabilityService_SeatMapAvailability_client_method.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv3_SeatMapAvailabilityService_SeatMapAvailability_server_method.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv3_SeatMapAvailabilityService_client.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv3_SeatMapAvailabilityService_server.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv3_SeatMapService_SeatMap_client_method.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv3_SeatMapService_SeatMap_server_method.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv3_SeatMapService_client.gois excluded by!**/generated/**internal/rpc/generated/seat_mapv3_SeatMapService_server.gois excluded by!**/generated/**internal/rpc/generated/transportv1_TransportSearchService_TransportSearch_server_method.gois excluded by!**/generated/**internal/rpc/generated/transportv1_TransportSearchService_client.gois excluded by!**/generated/**internal/rpc/generated/transportv1_TransportSearchService_server.gois excluded by!**/generated/**internal/rpc/generated/transportv2_TransportSearchService_TransportSearch_server_method.gois excluded by!**/generated/**internal/rpc/generated/transportv2_TransportSearchService_client.gois excluded by!**/generated/**internal/rpc/generated/transportv2_TransportSearchService_server.gois excluded by!**/generated/**internal/rpc/generated/transportv3_TransportProductListService_TransportProductList_client_method.gois excluded by!**/generated/**internal/rpc/generated/transportv3_TransportProductListService_TransportProductList_server_method.gois excluded by!**/generated/**internal/rpc/generated/transportv3_TransportProductListService_client.gois excluded by!**/generated/**internal/rpc/generated/transportv3_TransportProductListService_server.gois excluded by!**/generated/**internal/rpc/generated/transportv3_TransportSearchService_TransportSearch_client_method.gois excluded by!**/generated/**internal/rpc/generated/transportv3_TransportSearchService_TransportSearch_server_method.gois excluded by!**/generated/**internal/rpc/generated/transportv3_TransportSearchService_client.gois excluded by!**/generated/**internal/rpc/generated/transportv3_TransportSearchService_server.gois excluded by!**/generated/**internal/rpc/generated/unmarshal.gois excluded by!**/generated/**tests/e2e/bot/generated/client.gois excluded by!**/generated/**
📒 Files selected for processing (13)
examples/rpc/partner-plugin/server.go(31 hunks)go.mod(2 hunks)internal/messaging/noop_response_handler.go(1 hunks)internal/messaging/processor.go(4 hunks)internal/messaging/processor_test.go(2 hunks)internal/messaging/response_handler.go(3 hunks)internal/rpc/rpc.go(1 hunks)internal/rpc/server/server.go(2 hunks)scripts/generate_grpc_service_handlers.sh(11 hunks)templates/client.go.tpl(0 hunks)templates/server.go.tpl(1 hunks)templates/server_local_on_chain_method.go.tpl(1 hunks)templates/server_p2p_method.go.tpl(1 hunks)
💤 Files with no reviewable changes (1)
- templates/client.go.tpl
🚧 Files skipped from review as they are similar to previous changes (8)
- internal/messaging/noop_response_handler.go
- internal/messaging/processor_test.go
- templates/server_p2p_method.go.tpl
- go.mod
- internal/messaging/response_handler.go
- templates/server_local_on_chain_method.go.tpl
- templates/server.go.tpl
- internal/rpc/rpc.go
🧰 Additional context used
🧠 Learnings (1)
examples/rpc/partner-plugin/server.go (1)
Learnt from: VjeraTurk
PR: chain4travel/camino-messenger-bot#89
File: pp-mock/handlers/accommodation/v3/accommodation_search.go:148-160
Timestamp: 2025-01-22T09:22:31.686Z
Learning: The AccommodationSearch service in v3 intentionally reuses the AccommodationSearchResult type from v2 as per the protocol definition. This is not a version compatibility issue.
🪛 GitHub Actions: CI
scripts/generate_grpc_service_handlers.sh
[warning] 131-131: SC2086 (info): Double quote to prevent globbing and word splitting. Did you mean: cp $SERVER_TEMPLATE "$SERVER_GEN_FILE"
[warning] 132-132: SC2086 (info): Double quote to prevent globbing and word splitting. Did you mean: sed -i "$GENERAL_PARAM_REPLACE" "$SERVER_GEN_FILE"
[warning] 133-133: SC2086 (info): Double quote to prevent globbing and word splitting. Did you mean: sed -i -e "s#{{TEMPLATE}}#$SERVER_TEMPLATE#g" "$SERVER_GEN_FILE"
[warning] 151-151: SC2086 (info): Double quote to prevent globbing and word splitting. Did you mean: cp $SERVER_LOCAL_ON_CHAIN_TEMPLATE "$METHOD_GEN_FILE"
[warning] 152-152: SC2086 (info): Double quote to prevent globbing and word splitting. Did you mean: sed -i "$GENERAL_PARAM_REPLACE" "$METHOD_GEN_FILE"
[warning] 153-153: SC2086 (info): Double quote to prevent globbing and word splitting. Did you mean: sed -i "$METHOD_PARAM_REPLACE" "$METHOD_GEN_FILE"
[warning] 154-154: SC2086 (info): Double quote to prevent globbing and word splitting. Did you mean: sed -i -e "s#{{TEMPLATE}}#$SERVER_LOCAL_ON_CHAIN_TEMPLATE#g" "$METHOD_GEN_FILE"
🔇 Additional comments (4)
internal/rpc/server/server.go (1)
108-127: LGTM! The P2P message handling implementation looks good.The method correctly:
- Sets up tracing with appropriate span name
- Processes metadata
- Handles errors
- Stamps metadata with checkpoint
internal/messaging/processor.go (2)
49-56: LGTM! The interface changes are well-structured.The interface clearly separates P2P and local message processing concerns.
134-144: LGTM! The P2P message processing implementation is correct.The method handles different message categories appropriately and maintains backward compatibility.
scripts/generate_grpc_service_handlers.sh (1)
8-17: LGTM! The template configuration is well-organized.The script clearly separates templates and output paths for P2P and local services.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
scripts/generate_grpc_service_handlers.sh (4)
61-93: P2P Client Code Generation with Template Substitutions
In the P2P branch, the script correctly generates the client file using the client template and then iterates over methods to generate individual client method files with the appropriate parameter replacements. Note that the usage of array variables (e.g.replace_paramsandmethod_params) is consistent here, although consider quoting variables like$CLIENT_TEMPLATEfor added safety against word splitting.
128-155: Local On-Chain Server Generation via Dedicated Templates
For routing whereROUTING == "local"andON_CHAIN == "true", the script generates the server file usingLOCAL_OUTPATHand processes method files with theSERVER_LOCAL_ON_CHAIN_TEMPLATE. Note that the sed commands here use the raw string variables (e.g.$GENERAL_PARAM_REPLACEand$METHOD_PARAM_REPLACE) rather than an array approach. For consistency and to avoid word splitting issues, consider unifying the method of substitution with the P2P branch.
304-309: Output Directory Management
The script cleans and recreates the output directories for both P2P and local on-chain services, ensuring a fresh workspace for generated files. While the commands are functionally correct, it would be prudent to quote the variables (e.g."$P2P_OUTPATH") to avoid potential word splitting if directory names ever include spaces.
324-347: Robust Detection of GOPATH
The logic for determiningGO_PATHis thorough: it first checks the environment variable, then falls back ongo env, and finally tries default locations. This multi-step approach is robust, though you might streamline by relying directly ongo env GOPATHin environments where it is supported.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.gitignore(1 hunks)scripts/generate_grpc_service_handlers.sh(11 hunks)
✅ Files skipped from review due to trivial changes (1)
- .gitignore
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Unit Tests
- GitHub Check: Static Analysis
- GitHub Check: Build docker image
🔇 Additional comments (14)
scripts/generate_grpc_service_handlers.sh (14)
8-9: Introduce Distinct Server Method Templates
The new variablesSERVER_P2P_METHOD_TEMPLATEandSERVER_LOCAL_ON_CHAIN_TEMPLATEclearly separate the templates for P2P and local on-chain service methods. This not only improves clarity but also enables differentiated code generation going forward.
11-12: Declare Separate Output Directories for P2P and Local On-Chain
By introducingP2P_OUTPATHandLOCAL_OUTPATH, the script now explicitly distinguishes between output paths for P2P and local on-chain services—even though both currently point to the same directory. This design choice facilitates future separation if necessary.
15-17: Update Registration and Unmarshalling File Paths
The updates toREGISTER_SERVICES_SERVER_FILE,REGISTER_SERVICES_CLIENT_FILE, andUNMARSHALING_FILEnow referenceP2P_OUTPATH, aligning these core generated files with the P2P routing logic. Ensure that similar distinctions are maintained for local on-chain handlers, if needed in later enhancements.
53-53: Include EMPTY_IMPORT in Parameter Replacement
Appending the substitution" -e s#{{EMPTY_IMPORT}}#$EMPTY_IMPORT#g"toGENERAL_PARAM_REPLACEallows conditional injection of an import for empty (placeholder) responses. This is a neat way to handle cases when the output type is"Empty".
59-59: Reset EMPTY_IMPORT Before Generating Code
SettingEMPTY_IMPORT=""immediately before generating client and server code ensures that any previous value does not inadvertently affect subsequent template replacements. This reset is important for predictable file generation.
95-127: P2P Server Generation Employs Updated Templates
The code in this section correctly creates the P2P server file usingSERVER_TEMPLATEand then iterates over its methods to generate corresponding server method files usingSERVER_P2P_METHOD_TEMPLATE. The substitution on line 126 now properly references the correct template variable. Ensure that the input templates are well maintained to match the sed-based substitutions.
159-181: Server Services Registration Function
Thegenerate_register_services_serverfunction correctly consolidates service registration for servers. The generated Go code encapsulates the list of services into a function, which is clear and straightforward. Minor spacing changes (as indicated by the tilde) improve readability.
183-212: Client Services Registration Function
Thegenerate_register_services_clientfunction builds the client services map effectively. The structure and use of echo to generate file content are clear. No functional issues are observed here.
214-251: Unmarshalling Function Generation
Thegenerate_unmarshallingfunction correctly generates a Go function to unmarshal protocol buffer messages. The iterative approach to import declarations and switch-case generation is well implemented. Consider adding comments or error handling if the proto schema evolves, though the current approach is acceptable.
253-301: E2E Bot Client Generation Function
This function constructs both the client structure and its initialization with dynamic field and constructor arrays. The logic for processing_FIELDS,_PACKAGES, and_TYPESis clear, and the generation of the Go code is well arranged.
389-401: Service Tag Extraction via Shell Parameter Expansion
The block extractingTAGS_LINEand parsingSERVICE_TYPE,ROUTING, andON_CHAINusing shell parameter expansion works as intended given the expected annotation format. However, its fragility increases if the annotation format changes. Consider enhancing robustness (e.g., with more specific grep patterns or default fallbacks) if tag absence is a possibility.
419-419: Extracting COMMON_TYPES_VERSION from PB File
Usinggrepwith a positive lookbehind/lookahead and then piping totail -n 1captures the intended version number. While this works given the current file format, be aware that any changes in the proto file structure could break this extraction.
458-461: Appending Unmarshal Methods for P2P Routing
Within the loop, for files matching the P2P routing, the script appends two entries toUNMARSHAL_METHODS(for Request and Response). The formatting (e.g.,"${SERVICE}V${VERSION:1}Request") appears correct. Just ensure that theVERSIONmanipulation functions as expected in various cases.
491-500: Final Generation and Formatting Pipeline
The sequence of function calls to generate registration files, unmarshalling, and the E2E bot client—followed by runninggofumpton all output directories—demonstrates a coherent end-to-end generation pipeline. The use ofgofumpthelps enforce consistent formatting across generated code.
|
@coderabbitai pause |
✅ Actions performedReviews paused. |
8fa3e25 to
b622567
Compare
1. Updates generating grpc service handlers
generate_grpc_service_handlers.shscript for automatic code generation has been updated to account for the services tags:productcore(irrelevant for this PR)p2plocalon-chain(irrelevant for this PR)For now - distinguishing between
p2pandlocalservicesSummary by CodeRabbit
New Features
Refactor
Chores
.gitignoreto include thebindirectory.Tests