MailboxRPC 2/7: contract + codegen + make rpc - #86
Conversation
Summary of ChangesHello @bhandras, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request establishes the foundational scaffolding for a robust RPC-over-mailbox transport layer. It introduces the necessary protobuf contracts, runtime interfaces, and code generation tooling to enable durable, retry-safe RPC communication with at-least-once delivery semantics and cursor-based acknowledgment. This work is a critical step towards enhancing the system's durability and reliability, aligning with ongoing efforts to improve the mailbox/RPC specification. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
ccbc319 to
d4f03e7
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the initial scaffolding for an RPC-over-mailbox transport layer. It includes the mailboxrpc runtime interface package, the mailboxpb public protobuf contract, and a protoc-gen-mailboxrpc plugin for generating typed client stubs and server registration helpers. The scripts/gen_protos.sh has been updated to integrate the new generator, ensuring proper installation and conditional exclusion for the mailboxpb package. The overall architecture appears robust and well-documented, laying a solid foundation for a durable and retry-safe RPC mechanism.
d4f03e7 to
e4343d5
Compare
bhandras
changed the title
bhandras
changed the title
aade4ce to
7f50436
Compare
7f50436 to
9890890
Compare
ba42f3c to
3a5dbec
Compare
9890890 to
8166d7c
Compare
|
@codex review pls |
|
To use Codex here, create a Codex account and connect to github. |
a6c6186 to
abdad2a
Compare
|
@bhandras, remember to re-request review from reviewers when ready |
| GoName: "Message", | ||
| } | ||
|
|
||
| g.P("// ", serviceGoName, |
There was a problem hiding this comment.
I think we can use go templates here to make it a bit more readable: https://github.com/lightninglabs/falafel/blob/1b3698ff9a08bc70fc284ee8cfd7ed109b5d74b6/templates.go#L100
8166d7c to
06821ef
Compare
|
@claude review this |
|
@codex review this |
|
Claude finished @Roasbeef's task in 2m 48s —— View job Code ReviewReviewing PR #86: MailboxRPC contract + codegen SummaryThis PR establishes solid foundational scaffolding for RPC-over-mailbox transport. The architecture is clean, the interfaces are well-thought-out, and the code generation approach is sound. I've identified several issues that should be addressed, primarily around code style conformance to the project's development guidelines. Critical IssuesNone - this is solid foundational work. Major Issues1. Code generator uses string concatenation instead of templates (generator.go:112+) As Roasbeef mentioned in the review comments, the generator should use Go templates for better readability and maintainability. The current approach with extensive Consider using 2. Missing function comments throughout (development_guidelines.md violation) Per the development guidelines: "At a minimum every function must be commented with its intended purpose." Several functions lack proper GoDoc comments:
Each should have a detailed comment explaining the purpose, assumptions, and any important details. See development_guidelines.md lines 19-63 for examples. 3. Function parameter wrapping issues (generator.go) Several function definitions violate the wrapping guidelines:
Per development_guidelines.md lines 405-453, function definitions must follow specific wrapping rules. Minor Issues / Style Violations4. Missing blank line after multi-line function declaration
// WRONG
func generateFile(plugin *protogen.Plugin, file *protogen.File,
cfg Config) error {
if !shouldGenerateFile(file, cfg) {
// RIGHT
func generateFile(plugin *protogen.Plugin, file *protogen.File,
cfg Config) error {
if !shouldGenerateFile(file, cfg) {5. Inconsistent code spacing (generator.go) The 6. Long lines in generator output (generator.go:112-114) The generated GoDoc comment is split across multiple g.P("// ", serviceGoName,
"MailboxClient is a typed mailbox RPC client ", "for ",
serviceGoName, ".",
)This generates: While the output is fine, the code would be cleaner with templates or at least consolidating the string parts. 7. Unused parameters in clientMethodComment (generator.go:224-225) _ = g
_ = serviceGoNameThese underscore assignments are unnecessary - just remove the unused parameters or use them in a more meaningful comment. The comment is too generic ("calls the X RPC") and doesn't add value beyond what the function signature already says. Documentation & Testing8. Test coverage is minimal The generator_test.go only has 2 tests:
Missing tests for:
Per CLAUDE.md: "Strive for near 90% test coverage where practical." 9. Missing integration/golden tests Consider adding golden tests that verify the complete generated output matches expected files. This would catch unintended changes to the generation logic. Protobuf Contract (mailbox.proto)10. Comments could be more detailed The proto file has good structural comments but could benefit from more details:
11. Consider versioning strategy The proto package is
Runtime Interfaces (mailbox/rpc/*)12. Excellent design - minor documentation improvements The interface design is clean and well-documented. Minor suggestions:
Build Integration (gen_protos.sh)13. Good integration, minor improvements
Generated Code Quality14. Generated code looks good Spot-checking
The main change to Recommendations
Positive Notes
This is solid foundational work. The main issues are code style conformance and the need for template-based generation as Roasbeef mentioned. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 06821ef30b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| g.P("\tif len(opts) > 0 {") | ||
| g.P("\t\topt = opts[0]") | ||
| g.P("\t}") |
There was a problem hiding this comment.
The generated client methods accept opts ...RPCOptions but only copy opts[0], so any additional option structs are silently ignored. In call sites that compose options (for example, adding headers in one helper and overriding correlation/idempotency in another), this drops settings without any error and can change request routing/dedup behavior in production. Either reject len(opts) > 1 explicitly or merge all provided options deterministically.
Useful? React with 👍 / 👎.
|
Claude finished @Roasbeef's task in 2m 18s —— View job Code Review CompleteReviewing PR #86: MailboxRPC contract + codegen SummaryThis PR establishes solid foundational scaffolding for RPC-over-mailbox transport. The architecture is clean and well-designed, with good separation of concerns between runtime interfaces ( However, there are several code style violations that need to be addressed per the development guidelines. Critical IssuesNone - This is solid foundational work with no bugs or architectural problems. Major Issues1. Function parameter wrapping violations (generator.go)Lines 30-31, 78-82, 106-107 Multiple function definitions violate the wrapping guidelines from // WRONG (current)
func generateFile(plugin *protogen.Plugin, file *protogen.File,
cfg Config) error {
func generateService(g *protogen.GeneratedFile,
svc *protogen.Service, serviceFQN string) error {
func buildServiceData(g *protogen.GeneratedFile,
svc *protogen.Service, serviceFQN string) serviceData {Per the guidelines: "lines should not end with an open parenthesis if the function definition isn't finished yet." Correct form: func generateFile(plugin *protogen.Plugin, file *protogen.File, cfg Config) (
error) {
// OR if first line fits:
func generateService(g *protogen.GeneratedFile, svc *protogen.Service,
serviceFQN string) error {Additionally, multi-line function declarations should have a blank line after the opening brace (development_guidelines.md lines 436-453): // WRONG (current - line 30)
func generateFile(plugin *protogen.Plugin, file *protogen.File,
cfg Config) error {
if !shouldGenerateFile(file, cfg) {
// RIGHT
func generateFile(plugin *protogen.Plugin, file *protogen.File,
cfg Config) error {
if !shouldGenerateFile(file, cfg) {2. Missing comprehensive function comments (generator.go)Lines 11, 29, 61, 78, 95, 104, 136 Per CLAUDE.md: "Every function and method (including unexported ones) must have a comment starting with the function/method name" and "Comments should explain how/why, not just what." Current comments are minimal and don't explain the "why" or important details: // INSUFFICIENT (line 11)
// Generate generates mailbox RPC stubs for all services in the protoc request.
func Generate(plugin *protogen.Plugin, cfg Config) error {Better: // Generate generates mailbox RPC stubs for all services in the protoc request.
// It iterates through each proto file that should be generated and creates
// corresponding *_mailboxrpc.pb.go files containing typed client/server stubs
// for RPC-over-mailbox transport. Services can be excluded via cfg.ExcludeService
// to prevent generating stubs for the mailbox transport itself.
func Generate(plugin *protogen.Plugin, cfg Config) error {Similarly, functions like 3. RPCOptions only consumes opts[0] (templates.go:104-106)Codex correctly identified this issue. The generated code accepts func (c *ArkServiceMailboxClient) GetInfo(..., opts ...rpc.RPCOptions) (...) {
var opt rpc.RPCOptions
if len(opts) > 0 {
opt = opts[0] // Silently ignores opts[1], opts[2], etc.
}This is problematic if callers accidentally pass multiple options structs - the extras are silently dropped. Solutions:
I recommend option 3 (non-variadic) unless there's a specific reason for variadic args. If variadic is needed, go with option 1 (reject multiple). Minor Issues4. Code organization - large function without stanzas (generator.go:78-93)The // CURRENT (dense, hard to skim)
func buildServiceData(...) serviceData {
const mailboxrpcPath = "..."
const protoPath = "..."
data := serviceData{
ServiceName: ...,
...
}
for _, m := range svc.Methods {
data.Methods = append(...)
}
return data
}
// BETTER (organized into stanzas)
func buildServiceData(...) serviceData {
// Define import paths for external dependencies that will be
// registered with the protogen file generator.
const mailboxrpcPath = "github.com/lightninglabs/darepo-client/" +
"mailbox/rpc"
const protoPath = "google.golang.org/protobuf/proto"
// Build the service-level template data, resolving all external
// identifiers to register necessary imports.
data := serviceData{
ServiceName: svc.GoName,
ServiceFQN: serviceFQN,
RPCClient: resolveIdent(g, mailboxrpcPath, "RPCClient"),
...
}
// Add method-level template data for each RPC method in the service.
for _, m := range svc.Methods {
data.Methods = append(data.Methods, methodData{
...
})
}
return data
}5. Test coverage could be expanded (generator_test.go)Per CLAUDE.md: "Strive for near 90% test coverage where practical." Current tests (
Suggestions:
This is less critical for a code generator since compilation of generated code will catch many issues, but better test coverage would help catch regressions. 6. Proto contract documentation could be more detailed (mailbox.proto)The proto file has good structural comments but could benefit from more operational details: Line 29 ( Line 42 ( Line 44 ( Line 81 ( These are minor documentation improvements that would help future implementers. Documentation & Architecture7. Excellent interface design (mailbox/rpc/*)The runtime interface design is clean, well-documented, and appropriately minimal. Good choices:
Minor suggestion: The relationship between
This is documented in options.go:13-14 but could be restated in the 8. Code generation approach is soundThe template-based approach (templates.go) is much better than the previous g.P() concatenation. Good decisions:
One observation: The 9. Build integration is clean (gen_protos.sh)The build integration is well-thought-out:
Minor suggestion: Consider adding error checking after Positive Notes✅ Template-based generation is much more maintainable than string concatenation Recommendations (Priority Order)
Should this PR be merged?Yes, with fixes for items 1-4 above. Items 5-8 are nice-to-haves but not blockers. This is solid foundational work that establishes a clean architecture for RPC-over-mailbox transport. The main issues are code style violations that should be caught by |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 25e96e5aba
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| RPCOptions: resolveIdent(g, mailboxrpcPath, "RPCOptions"), | ||
| Router: resolveIdent(g, mailboxrpcPath, "Router"), | ||
| Context: resolveIdent(g, "context", "Context"), | ||
| ProtoMessage: resolveIdent(g, protoPath, "Message"), | ||
| FmtErrorf: resolveIdent(g, "fmt", "Errorf"), |
There was a problem hiding this comment.
buildServiceData always registers RPCOptions, context.Context, proto.Message, and fmt.Errorf imports, but the template only emits those identifiers inside {{range .Methods}} sections. For a valid proto service with zero RPC methods, the generated *_mailboxrpc.pb.go file will still carry these imports even though no code references them, causing a compile-time unused-import failure. Please gate these resolveIdent calls on len(svc.Methods) > 0 (or resolve lazily per method) so empty-service generation remains buildable.
Useful? React with 👍 / 👎.
Replace g.P() call chains with a text/template in the protoc plugin. This makes the generated output shape directly visible in the template text, following the pattern used by falafel. Split generateService into modular helpers: resolveIdent registers imports and returns qualified names, buildServiceData constructs template data from protogen types, and generateService orchestrates template execution into a buffer before writing to g.
25e96e5 to
d5fb2f9
Compare
3 participants
Summary
This PR adds initial scaffolding for an RPC-over-mailbox transport layer:
mailboxrpc: a small runtime interface package for sending/awaiting RPCs and registering handlers.mailboxpb: the public protobuf contract for the mailbox edge transport.protoc-gen-mailboxrpc: a repo-local protoc plugin that generates typed RPC-over-mailbox client stubs and server registration helpers.make rpcwiring to generate mailbox RPC stubs alongside existing protos.Why
We want a durable, retry-safe RPC layer that can be transported over a mailbox edge with at-least-once delivery semantics and cursor-based acking. This is part of the durability work tracked in #48 and the mailbox/RPC spec tracked in lightninglabs/darepo#71.
Notes
*.pb.goand*_mailboxrpc.pb.gofiles are checked in.make rpc(Docker-based tooling).Follow-ups
mailboxrpc.RPCClientand a router backed by the mailbox transport.