Skip to content

Bulletin Pulsar update#103

Closed
JesseAbram wants to merge 1 commit intodevfrom
jesse/pulsar_updates
Closed

Bulletin Pulsar update#103
JesseAbram wants to merge 1 commit intodevfrom
jesse/pulsar_updates

Conversation

@JesseAbram
Copy link
Copy Markdown
Contributor

These were not generated due to me not having protoc-gen-go-grpc

@JesseAbram JesseAbram requested a review from iverc March 31, 2026 17:10
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Mar 31, 2026

📝 Walkthrough

Walkthrough

Added artifact string field to EventPostCreated and MsgCreatePost protobuf messages with corresponding Go struct fields and getter methods. Introduced new protobuf message types QueryBulletinPolicyIdRequest and QueryBulletinPolicyIdResponse with reflection and serialization support. Updated message descriptor indices and service method wiring.

Changes

Cohort / File(s) Summary
Artifact field additions
api/sourcehub/bulletin/events.pulsar.go, api/sourcehub/bulletin/tx.pulsar.go
Added optional artifact string field (protobuf field 5) to both EventPostCreated and MsgCreatePost messages. Extended fast reflection implementations with field descriptor logic, updated serialization/deserialization in ProtoMethods, and added GetArtifact() getter methods.
Query message types
api/sourcehub/bulletin/query.pulsar.go
Added new QueryBulletinPolicyIdRequest and QueryBulletinPolicyIdResponse message types with full protobuf reflection and marshal/unmarshal support. Updated message descriptor indices from 16 total to 18, shifting QueryIterateGlobRequest/Response indices from 14/15 to 16/17. Wired new service method in descriptor depIdxs.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

  • Add artifact to create post event #93: Adds the same artifact protobuf field to post-related messages with corresponding Go code updates and usage patterns.
  • Add bulletin acp rpc #97: Introduces the Query.BulletinPolicyId RPC endpoint that directly corresponds to the new QueryBulletinPolicyIdRequest/Response message types in this PR.

Suggested reviewers

  • iverc
  • Lodek

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@JesseAbram JesseAbram requested a review from Lodek March 31, 2026 17:10
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
api/sourcehub/bulletin/tx.pulsar.go (1)

4861-4917: ⚠️ Potential issue | 🔴 Critical

artifact is accepted but not persisted.

The MsgCreatePost message (line 4861) accepts an artifact field, but the generated code and keeper implementation show a critical mismatch: artifact is emitted only in EventPostCreated and never stored in the Post model. The Post struct in x/bulletin/types/post.pb.go has no artifact field, and CreatePost in x/bulletin/keeper/msg_server.go drops the artifact when persisting (lines 109–114), only forwarding it to the event (line 123). Clients can submit this field and see it in the event stream, but any later post retrieval loses it. Add artifact to the Post message definition, persist it in CreatePost, and regenerate the protobuf outputs.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@api/sourcehub/bulletin/tx.pulsar.go` around lines 4861 - 4917, The Post
artifact field is accepted in MsgCreatePost and emitted in EventPostCreated but
never persisted; update the Post protobuf message (the Post struct in
x/bulletin/types/post.proto/.pb.go) to include an artifact string field, modify
the CreatePost handler in x/bulletin/keeper/msg_server.go to copy msg.Artifact
into the stored Post before saving, and then regenerate protobuf code so
MsgCreatePost, Post, and related .pb.go files are consistent and the artifact is
persisted and returned on post retrieval.
🧹 Nitpick comments (1)
api/sourcehub/bulletin/events.pulsar.go (1)

2370-2427: Add one round-trip regression test for Artifact.

Given the PR note about the unavailable generator toolchain, a small marshal/unmarshal assertion for EventPostCreated{Artifact: "..."} would be a cheap guard against future drift between the struct, reflection path, and raw descriptor.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@api/sourcehub/bulletin/events.pulsar.go` around lines 2370 - 2427, Add a
regression test that ensures EventPostCreated.Artifact survives protobuf
marshal/unmarshal round-trips: create an instance of EventPostCreated with
Artifact set (e.g., "test-artifact"), marshal it with proto.Marshal (using the
generated message type EventPostCreated), then unmarshal into a new
EventPostCreated and assert the Artifact field equals the original; use the
generated methods (proto.Marshal/proto.Unmarshal and EventPostCreated) and
include this test alongside other message tests so future changes to the struct,
Descriptor, or reflection path are caught.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@api/sourcehub/bulletin/tx.pulsar.go`:
- Around line 4861-4917: The Post artifact field is accepted in MsgCreatePost
and emitted in EventPostCreated but never persisted; update the Post protobuf
message (the Post struct in x/bulletin/types/post.proto/.pb.go) to include an
artifact string field, modify the CreatePost handler in
x/bulletin/keeper/msg_server.go to copy msg.Artifact into the stored Post before
saving, and then regenerate protobuf code so MsgCreatePost, Post, and related
.pb.go files are consistent and the artifact is persisted and returned on post
retrieval.

---

Nitpick comments:
In `@api/sourcehub/bulletin/events.pulsar.go`:
- Around line 2370-2427: Add a regression test that ensures
EventPostCreated.Artifact survives protobuf marshal/unmarshal round-trips:
create an instance of EventPostCreated with Artifact set (e.g.,
"test-artifact"), marshal it with proto.Marshal (using the generated message
type EventPostCreated), then unmarshal into a new EventPostCreated and assert
the Artifact field equals the original; use the generated methods
(proto.Marshal/proto.Unmarshal and EventPostCreated) and include this test
alongside other message tests so future changes to the struct, Descriptor, or
reflection path are caught.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: d43dd738-97f2-4f18-b76b-4358c610b27e

📥 Commits

Reviewing files that changed from the base of the PR and between 3f133bf and 54aefaf.

⛔ Files ignored due to path filters (1)
  • api/sourcehub/bulletin/query_grpc.pb.go is excluded by !**/*.pb.go
📒 Files selected for processing (3)
  • api/sourcehub/bulletin/events.pulsar.go
  • api/sourcehub/bulletin/query.pulsar.go
  • api/sourcehub/bulletin/tx.pulsar.go
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: test
🔇 Additional comments (4)
api/sourcehub/bulletin/events.pulsar.go (1)

586-1220: LGTM: artifact is plumbed consistently across the generated message.

Descriptor lookup, reflection helpers, size/marshal/unmarshal, the exported field, getter, and raw descriptor bytes all agree on a length-delimited string at field 5. That also matches the typed event shape in x/bulletin/types/events.pb.go:95-106.

Also applies to: 2370-2427, 2549-2587

api/sourcehub/bulletin/query.pulsar.go (3)

6744-7518: QueryBulletinPolicyId* matches the proto contract.

The empty request and single policy_id response are wired consistently through the raw descriptor, fast-reflection helpers, concrete structs, and getter.

Also applies to: 9209-9270, 9487-9493


7543-7545: The message-slot renumbering is internally consistent.

The promotion of QueryBulletinPolicyId* to slots 14/15 and the resulting move of QueryIterateGlob* to 16/17 is reflected coherently in the descriptor bytes, reset/descriptor paths, type metadata, dependency indexes, exporter slots, and message count.

Also applies to: 8155-8157, 9284-9301, 9335-9352, 9494-9511, 9631-9684, 9880-9935


9512-9616: No action needed—the generated RPC stubs are complete.

BulletinPolicyId is already fully exposed in query_grpc.pb.go, including the client method, server interface, unimplemented base, handler, and service registration. The descriptor in query.pulsar.go aligns with the callable transport layer.

			> Likely an incorrect or invalid review comment.

@codecov-commenter
Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 47.86%. Comparing base (6393001) to head (54aefaf).
⚠️ Report is 2 commits behind head on dev.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #103      +/-   ##
==========================================
+ Coverage   47.80%   47.86%   +0.05%     
==========================================
  Files         276      276              
  Lines       16196    16208      +12     
==========================================
+ Hits         7743     7758      +15     
+ Misses       7647     7645       -2     
+ Partials      806      805       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@JesseAbram JesseAbram closed this Apr 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants