Migrate to buf v2 #694
Migrate to buf v2 #694
Conversation
WalkthroughThis PR updates build/protobuf tooling to Buf v2 using official bufbuild images, removes custom Buf Docker setup, and revises OpenAPI specs. Error schemas switch from rpcStatus to googlerpcStatus across ark specs, wallet schemas are renamed/restructured (arkv1*/arkwalletv1*), and an e2e test adds a brief synchronization delay. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (9)
api-spec/openapi/swagger/arkwallet/v1/bitcoin_wallet.swagger.json (1)
959-1031: New arkwalletv1 definitions added — consider adding “required” fields for better client validation*The new definitions look fine. As an optional improvement, explicitly mark required fields so clients can validate earlier:
- arkwalletv1CreateRequest: seed, password
- arkwalletv1RestoreRequest: seed, password
- arkwalletv1UnlockRequest: password
- arkwalletv1WithdrawRequest: address, amount
Here’s a minimal diff illustrating the change:
"arkwalletv1CreateRequest": { "type": "object", + "required": ["seed", "password"], "properties": { "seed": { "type": "string" }, "password": { "type": "string" } } }, "arkwalletv1RestoreRequest": { "type": "object", + "required": ["seed", "password"], "properties": { "seed": { "type": "string" }, "password": { "type": "string" } } }, "arkwalletv1UnlockRequest": { "type": "object", + "required": ["password"], "properties": { "password": { "type": "string" } } }, "arkwalletv1WithdrawRequest": { "type": "object", + "required": ["address", "amount"], "properties": { "address": { "type": "string" }, "amount": { "type": "string", "format": "uint64" } } }api-spec/openapi/swagger/ark/v1/wallet.swagger.json (4)
359-361: Optional: add a bounded array size to satisfy static analysisCheckov flagged arrays without maxItems. If acceptable for your API, add a reasonable bound to details to quiet the linter.
Proposed change:
"details": { - "type": "array", + "type": "array", + "maxItems": 128, "items": { "$ref": "#/definitions/protobufAny" } }If an upper bound is inappropriate here, consider suppressing CKV_OPENAPI_21 for this definition.
306-308: Mark password fields with format: passwordAnnotating password fields improves client UX (masking) and communicates sensitivity in tooling.
Apply:
"password": { - "type": "string" + "type": "string", + "format": "password" }This applies to:
- arkv1CreateRequest.properties.password (Line 279)
- arkv1RestoreRequest.properties.password (Line 306)
- arkv1UnlockRequest.properties.password (Line 321)
Also applies to: 279-280, 321-323
310-312: Constrain numeric-string fields with a regex patterngapLimit and amount are strings with uint64 format. Add a numeric pattern to enable validation without changing types.
Apply:
"gapLimit": { "type": "string", - "format": "uint64" + "format": "uint64", + "pattern": "^[0-9]+$" }"amount": { "type": "string", - "format": "uint64" + "format": "uint64", + "pattern": "^[0-9]+$" }Also applies to: 336-337
275-281: Security check: sensitive fields exposureThe API accepts/returns seed and password in several schemas. Ensure transport is always over TLS, and verify logging/metrics don’t capture these fields. Consider marking these properties with x-sensitive or vendor extensions supported by your tooling to prevent accidental leakage.
If you share your OpenAPI toolchain, I can suggest the appropriate vendor extensions or filters to mask sensitive fields in generated docs and logs.
Also applies to: 289-293, 303-314, 330-340, 344-348
test/e2e/e2e_test.go (1)
333-334: Replace fixed sleep with a confirmation-aware wait to deflake the test.A hardcoded 2s delay is brittle under load/CI variance. Poll for readiness instead.
Apply this diff within the selected lines:
- time.Sleep(2 * time.Second) // give time to process confirmation + require.True(t, waitFor(func() bool { + // Cheap readiness probe – any successful call implies the server processed recent blocks. + _, err := runArkCommand("balance") + return err == nil + }, 10*time.Second, 200*time.Millisecond), "timeout waiting for confirmations to be processed")Add this helper somewhere in the file (outside the selected range):
// waitFor polls cond every interval until it returns true or timeout elapses. func waitFor(cond func() bool, timeout, interval time.Duration) bool { deadline := time.Now().Add(timeout) for { if cond() { return true } if time.Now().After(deadline) { return false } time.Sleep(interval) } }buf.yaml (1)
5-6: Pin dependencies by committing buf.lock for reproducible builds.With deps specified, ensure a buf.lock is generated and committed so CI/devs resolve the same digests over time.
If not present, run the appropriate Buf command locally to generate the lockfile (Buf v2: buf dep update) and commit buf.lock. Please confirm it’s included in the PR.
Makefile (1)
139-139: Pin Buf CLI image and avoid root-owned artifacts.
- Using an untagged image risks non-reproducible builds.
- Without --user, generated files may be owned by root on Linux hosts.
Suggest pinning the image via a variable and running as the current user.
Apply this diff within the selected lines:
- @docker run --rm --volume "$(shell pwd):/workspace" --workdir /workspace bufbuild/buf generate + @docker run --rm --user "$(shell id -u):$(shell id -g)" --volume "$(shell pwd):/workspace" --workdir /workspace $(BUF_IMAGE) generate- @docker run --rm --volume "$(shell pwd):/workspace" --workdir /workspace bufbuild/buf lint + @docker run --rm --user "$(shell id -u):$(shell id -g)" --volume "$(shell pwd):/workspace" --workdir /workspace $(BUF_IMAGE) lintAdd this near the top of the Makefile (outside the selected range):
# Pin the Buf CLI image to ensure reproducible builds (choose the version you support) BUF_IMAGE ?= bufbuild/buf:1.51.0Also applies to: 145-145
buf.gen.yaml (1)
21-22: Add a newline at EOF to satisfy yamllint.Current file is missing a trailing newline.
Apply this diff to append a newline (no functional change):
- remote: buf.build/grpc-ecosystem/openapiv2:v2.27.1 out: api-spec/openapi/swagger +
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (18)
api-spec/protobuf/buf.lockis excluded by!**/*.lockapi-spec/protobuf/gen/ark/v1/admin.pb.gois excluded by!**/*.pb.go,!**/gen/**api-spec/protobuf/gen/ark/v1/admin.pb.gw.gois excluded by!**/*.pb.gw.go,!**/gen/**api-spec/protobuf/gen/ark/v1/admin_grpc.pb.gois excluded by!**/*.pb.go,!**/gen/**api-spec/protobuf/gen/ark/v1/indexer.pb.gois excluded by!**/*.pb.go,!**/gen/**api-spec/protobuf/gen/ark/v1/indexer.pb.gw.gois excluded by!**/*.pb.gw.go,!**/gen/**api-spec/protobuf/gen/ark/v1/indexer_grpc.pb.gois excluded by!**/*.pb.go,!**/gen/**api-spec/protobuf/gen/ark/v1/service.pb.gois excluded by!**/*.pb.go,!**/gen/**api-spec/protobuf/gen/ark/v1/service.pb.gw.gois excluded by!**/*.pb.gw.go,!**/gen/**api-spec/protobuf/gen/ark/v1/service_grpc.pb.gois excluded by!**/*.pb.go,!**/gen/**api-spec/protobuf/gen/ark/v1/types.pb.gois excluded by!**/*.pb.go,!**/gen/**api-spec/protobuf/gen/ark/v1/wallet.pb.gois excluded by!**/*.pb.go,!**/gen/**api-spec/protobuf/gen/ark/v1/wallet.pb.gw.gois excluded by!**/*.pb.gw.go,!**/gen/**api-spec/protobuf/gen/ark/v1/wallet_grpc.pb.gois excluded by!**/*.pb.go,!**/gen/**api-spec/protobuf/gen/arkwallet/v1/bitcoin_wallet.pb.gois excluded by!**/*.pb.go,!**/gen/**api-spec/protobuf/gen/arkwallet/v1/bitcoin_wallet.pb.gw.gois excluded by!**/*.pb.gw.go,!**/gen/**api-spec/protobuf/gen/arkwallet/v1/bitcoin_wallet_grpc.pb.gois excluded by!**/*.pb.go,!**/gen/**buf.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
Makefile(1 hunks)api-spec/openapi/swagger/ark/v1/admin.swagger.json(10 hunks)api-spec/openapi/swagger/ark/v1/indexer.swagger.json(15 hunks)api-spec/openapi/swagger/ark/v1/service.swagger.json(15 hunks)api-spec/openapi/swagger/ark/v1/types.swagger.json(2 hunks)api-spec/openapi/swagger/ark/v1/wallet.swagger.json(15 hunks)api-spec/openapi/swagger/arkwallet/v1/bitcoin_wallet.swagger.json(12 hunks)api-spec/protobuf/buf.yaml(0 hunks)buf.Dockerfile(0 hunks)buf.gen.yaml(1 hunks)buf.work.yaml(0 hunks)buf.yaml(1 hunks)test/e2e/e2e_test.go(1 hunks)
💤 Files with no reviewable changes (3)
- buf.Dockerfile
- buf.work.yaml
- api-spec/protobuf/buf.yaml
🧰 Additional context used
🪛 YAMLlint (1.37.1)
buf.gen.yaml
[error] 22-22: no new line character at the end of file
(new-line-at-end-of-file)
🪛 Checkov (3.2.334)
api-spec/openapi/swagger/ark/v1/wallet.swagger.json
[MEDIUM] 359-365: Ensure that arrays have a maximum number of items
(CKV_OPENAPI_21)
🔇 Additional comments (15)
api-spec/openapi/swagger/arkwallet/v1/bitcoin_wallet.swagger.json (6)
154-171: Create endpoint now references arkwalletv1 types — good migration*Both request and response schemas correctly switched to arkwalletv1CreateRequest/Response.
316-333: Lock endpoint migrated to arkwalletv1 — consistent with the new naming*References updated to arkwalletv1LockRequest/Response as expected.
508-525: Restore endpoint migrated to arkwalletv1 — looks correct*Request/response schemas reference arkwalletv1RestoreRequest/Response.
540-541: Seed endpoint response updated to arkwalletv1GenSeedResponse — OKConsistent with the new arkwalletv1* group.
772-789: Unlock endpoint migrated to arkwalletv1 — OK*Schema references updated to arkwalletv1UnlockRequest/Response.
932-949: Withdraw endpoint migrated to arkwalletv1 — OK*Schema references updated to arkwalletv1WithdrawRequest/Response.
api-spec/openapi/swagger/ark/v1/types.swagger.json (1)
15-33: No leftover rpcStatus references; googlerpcStatus rename verified
- Confirmed no occurrences of
rpcStatusin definitions or$refacross the repo- Default error schemas consistently reference
#/definitions/googlerpcStatusprotobufAnyremains correctly used for thedetailsarrayApproving these changes.
api-spec/openapi/swagger/ark/v1/indexer.swagger.json (1)
33-34: Systematic migration to googlerpcStatus — looks correct and consistent
- All default error responses and streaming “error” wrappers now point to googlerpcStatus.
- googlerpcStatus is defined locally and protobufAny remains available.
No issues spotted with the replacement.
Also applies to: 71-72, 123-124, 175-176, 206-207, 296-297, 329-330, 338-339, 402-403, 452-453, 504-505, 574-592, 593-601
api-spec/openapi/swagger/ark/v1/service.swagger.json (1)
33-34: Error schema migration to googlerpcStatus across service endpoints — consistent and correct
- Default errors and streaming “error” fields reference googlerpcStatus everywhere you modified.
- Definitions include googlerpcStatus and protobufAny.
All good.
Also applies to: 99-100, 108-109, 143-144, 176-177, 209-210, 242-243, 275-276, 298-299, 364-365, 373-374, 384-402, 403-411
api-spec/openapi/swagger/ark/v1/admin.swagger.json (1)
32-33: Admin API error schema migration — consistent use of googlerpcStatus
- All updated default error references point to googlerpcStatus.
- Definition added and protobufAny retained.
Looks good.
Also applies to: 66-67, 98-99, 118-119, 150-151, 182-183, 212-213, 250-251, 261-279, 280-288
api-spec/openapi/swagger/ark/v1/wallet.swagger.json (3)
35-35: Consistent migration of error schema to googlerpcStatus looks goodDefault error responses across endpoints now reference googlerpcStatus consistently.
Also applies to: 57-57, 79-79, 111-111, 143-143, 175-175, 197-197, 219-219, 251-251
73-74: Verify server and clients are regenerated for arkv1 renames*Request/response refs for wallet endpoints were renamed to arkv1* types. Ensure:
- Server handlers bind to these new protobuf/OpenAPI symbols.
- Generated SDKs/clients are updated and any downstream integrations are adjusted.
Do you want a quick script to scan the repo for old v1* symbol usages and suggest updates?
Also applies to: 89-90, 105-106, 121-122, 137-138, 153-154, 169-170, 213-214, 229-230, 245-246, 261-262
191-192: Confirm 200 schema for GetStatus remains v1GetStatusResponse (vs. googlerpcStatus)Success schema here still references v1GetStatusResponse while defaults use googlerpcStatus. If the intent was to return googlerpcStatus for success as well, update this reference; otherwise, keep as-is.
I can generate a repo-wide check to see if any other status endpoints switched to googlerpcStatus so we keep this consistent—want me to run that?
buf.yaml (1)
1-21: LGTM: Buf v2 module config is correct and minimal.
- Root-level v2 config, single module path/name, googleapis dep, sensible lint rules, and FILE-level breaking checks all look good.
buf.gen.yaml (1)
1-22: LGTM: v2 gen config with managed mode and pinned remote plugins.
- clean: true is appropriate for fully generated dirs.
- Managed overrides look correct (prefix set, googleapis go_package disabled).
- Remote plugins are version-pinned.
|
does this work with upcoming OpenAPI v3? |
cc @louisinger |
|
yes v2 is just another file syntax but plugins are the same |
|
@louisinger did you verify if ther's a remote plugin for grpc-api-gateway? |
This PR runs the
buf config migrateCLI command to migrate from v1 to v2.see v2 blogpost for the "why?": https://buf.build/blog/buf-cli-next-generation
v2 is backward compatible with v1 so it's not a breaking change. It allows simplifying the buf configuration by removing
buf.work.yamlfile and movingbuf.yamlto main folder.This PR also use remote plugins instead of a custom
buf.Dockerfileimage. Now the imagebufbuild/bufcan be used to generate proto stubs without any extra step.@sekulicd @Kukks @altafan please review
Summary by CodeRabbit
Refactor
Chores
Tests