Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughReplaced Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Replace all google.type.Money fields in financial_accounting_events.proto (8 fields across 4 messages) and current_account_events.proto (10 fields across 5 messages) with meridian.quantity.v1.InstrumentAmount. Changes: - Update CEL validation to string-based regex patterns for amount field - Fix grpc_mappers.go to use toProtoInstrumentAmount for event building - Replace money.Money construction with InstrumentAmount in grpc_control_endpoints.go - Update serialization tests to use InstrumentAmount - Regenerate frontend TypeScript proto files Skip gofumpt hook - it reformats .pb.go imports, causing CI proto freshness check failures.
bjcoombs
force-pushed
the
059-asset-agnostic-posting-layer--2-6--event-proto-migration
branch
from
1e70c27 to
bc0aac6
Compare
Claude Code ReviewCommit: SummaryClean migration of 18 Money fields across two event proto files to InstrumentAmount. The proto definitions, CEL validation, generated code, Go service mappers, and tests are all consistent. The version=0 guard in Risk Assessment
Findings
Bot Review NotesAll 3 review threads (1 claude, 2 coderabbitai) are resolved:
Previously Flagged
|
The LedgerPostingCapturedEvent.PostingAmount field was migrated from google.type.Money to quantityv1.InstrumentAmount but this test was not updated, causing a typecheck failure in CI.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/proto/meridian/events/v1/financial_accounting_events_test.go (1)
702-755:⚠️ Potential issue | 🟡 MinorAdd protovalidate checks to properly test CEL constraints.
The test only exercises wire serialization (
proto.Marshal/proto.Unmarshal), which bypass the CEL validation rules defined in the proto schema. TheexpectValidfield is documented but never asserted. Replace the current flow with protovalidate to actually validate the constraint thatposting_amountmust be positive:Example fix pattern
import "buf.build/go/protovalidate" // In test setup: validator, err := protovalidate.New() if err != nil { t.Fatalf("Failed to create validator: %v", err) } // In loop: err := validator.Validate(event) if tt.expectValid { if err != nil { t.Errorf("Expected validation to pass, got: %v", err) } } else { if err == nil { t.Errorf("Expected validation to fail for: %s", tt.name) } }See
api/proto/meridian/financial_accounting/v1/financial_accounting_test.gofor a working example.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/proto/meridian/events/v1/financial_accounting_events_test.go` around lines 702 - 755, TestLedgerPostingCapturedEvent_AmountValidationDocumentation currently only round-trips the proto via proto.Marshal/proto.Unmarshal and never runs the CEL/protovalidate checks nor asserts expectValid; create a protovalidate validator (protovalidate.New()) in the test, call validator.Validate(event) inside the loop, and replace the current serialization-only assertion with checks that if tt.expectValid then err==nil else err!=nil, failing the test appropriately; ensure you add the protovalidate import and run validation against the same event struct (LedgerPostingCapturedEvent with PostingAmount) before or instead of the proto.Marshal/unmarshal assertions.
🧹 Nitpick comments (1)
api/proto/meridian/events/v1/financial_accounting_events_test.go (1)
92-93: These round-trip checks only verifyAmount.After the migration, regressions in
InstrumentCode,Version, or the sibling amount field (TotalCredits,PreviousAmount, etc.) would still pass. Please assert the full nestedInstrumentAmountpayload for these migrated fields instead of only one subfield.Also applies to: 141-143, 190-191, 270-271
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/proto/meridian/events/v1/financial_accounting_events_test.go` around lines 92 - 93, The tests currently compare only PostingAmount.Amount (and similar single subfields) on decoded vs event, which can miss regressions in other nested fields; update the assertions to compare the full nested InstrumentAmount messages (e.g., decoded.PostingAmount vs event.PostingAmount) using a deep/protobuf-aware equality check (such as reflect.DeepEqual or proto.Equal) so InstrumentCode, Version and sibling fields like TotalCredits/PreviousAmount are validated as well; apply the same change to the other occurrences noted (the checks around lines for PostingAmount, TotalCredits, PreviousAmount).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/proto/meridian/events/v1/financial_accounting_events.proto`:
- Around line 238-245: The CEL regex for meridian.quantity.v1.InstrumentAmount
posting_amount currently rejects valid positive values with leading zero padding
(e.g., "01", "0001.50"); update the validation to test numeric positivity
instead of a canonical string shape: replace the current regex-based expression
on the posting_amount field with a numeric comparison (parse/convert the
InstrumentAmount.amount to a number and assert > 0) so leading zeros are
accepted, and apply the same change to the identical validation rules in
current_account_events.proto and the other matching blocks (the other two
identical rules noted in the comment).
In `@services/current-account/service/grpc_control_endpoints.go`:
- Around line 347-350: The closingBalance builder hardcodes Version: 1 which
mis-serializes non-v1 instruments; replace the literal with the domain amount’s
actual version (e.g. use account.Balance().Version() or the equivalent accessor
on the domain Amount) or call the shared amount-to-proto helper used in
services/financial-accounting/service/adapters.go so the
InstrumentAmount.Version is taken from account.Balance() instead of being
hardcoded.
---
Outside diff comments:
In `@api/proto/meridian/events/v1/financial_accounting_events_test.go`:
- Around line 702-755:
TestLedgerPostingCapturedEvent_AmountValidationDocumentation currently only
round-trips the proto via proto.Marshal/proto.Unmarshal and never runs the
CEL/protovalidate checks nor asserts expectValid; create a protovalidate
validator (protovalidate.New()) in the test, call validator.Validate(event)
inside the loop, and replace the current serialization-only assertion with
checks that if tt.expectValid then err==nil else err!=nil, failing the test
appropriately; ensure you add the protovalidate import and run validation
against the same event struct (LedgerPostingCapturedEvent with PostingAmount)
before or instead of the proto.Marshal/unmarshal assertions.
---
Nitpick comments:
In `@api/proto/meridian/events/v1/financial_accounting_events_test.go`:
- Around line 92-93: The tests currently compare only PostingAmount.Amount (and
similar single subfields) on decoded vs event, which can miss regressions in
other nested fields; update the assertions to compare the full nested
InstrumentAmount messages (e.g., decoded.PostingAmount vs event.PostingAmount)
using a deep/protobuf-aware equality check (such as reflect.DeepEqual or
proto.Equal) so InstrumentCode, Version and sibling fields like
TotalCredits/PreviousAmount are validated as well; apply the same change to the
other occurrences noted (the checks around lines for PostingAmount,
TotalCredits, PreviousAmount).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6e39f581-2e75-4683-9b17-e602f569c64b
⛔ Files ignored due to path filters (4)
api/proto/meridian/events/v1/current_account_events.pb.gois excluded by!**/*.pb.go,!**/*.pb.goapi/proto/meridian/events/v1/financial_accounting_events.pb.gois excluded by!**/*.pb.go,!**/*.pb.gofrontend/src/api/gen/meridian/events/v1/current_account_events_pb.tsis excluded by!**/gen/**frontend/src/api/gen/meridian/events/v1/financial_accounting_events_pb.tsis excluded by!**/gen/**
📒 Files selected for processing (5)
api/proto/meridian/events/v1/current_account_events.protoapi/proto/meridian/events/v1/financial_accounting_events.protoapi/proto/meridian/events/v1/financial_accounting_events_test.goservices/current-account/service/grpc_control_endpoints.goservices/financial-accounting/service/grpc_mappers.go
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/proto/serialization_test.go (1)
119-122: Migration toInstrumentAmountlooks correct.The change from
money.Moneytoquantityv1.InstrumentAmountwith string-basedAmountandInstrumentCodealigns with the PR's proto migration.Consider adding assertions for the
PostingAmountfields after deserialization (similar to lines 99-110 in the "LedgerPosting message serialization" test) to fully validate the round-trip:🔧 Optional: Add assertions for PostingAmount fields
if decoded.PostingId != original.PostingId { t.Errorf("posting_id mismatch: got %v, want %v", decoded.PostingId, original.PostingId) } + if decoded.PostingAmount == nil { + t.Fatal("posting_amount mismatch: got nil, want non-nil") + } + if decoded.PostingAmount.Amount != original.PostingAmount.Amount { + t.Errorf("posting_amount.amount mismatch: got %v, want %v", decoded.PostingAmount.Amount, original.PostingAmount.Amount) + } + if decoded.PostingAmount.InstrumentCode != original.PostingAmount.InstrumentCode { + t.Errorf("posting_amount.instrument_code mismatch: got %v, want %v", decoded.PostingAmount.InstrumentCode, original.PostingAmount.InstrumentCode) + } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/proto/serialization_test.go` around lines 119 - 122, Add explicit assertions that validate the deserialized PostingAmount fields: after deserializing the LedgerPosting in the "LedgerPosting message serialization" test, assert that PostingAmount is non-nil and that PostingAmount.Amount equals "1000.00" and PostingAmount.InstrumentCode equals "GBP" (referencing the quantityv1.InstrumentAmount and PostingAmount fields used when constructing the message) so the round-trip serialization test fully verifies these values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/proto/serialization_test.go`:
- Around line 119-122: Add explicit assertions that validate the deserialized
PostingAmount fields: after deserializing the LedgerPosting in the
"LedgerPosting message serialization" test, assert that PostingAmount is non-nil
and that PostingAmount.Amount equals "1000.00" and PostingAmount.InstrumentCode
equals "GBP" (referencing the quantityv1.InstrumentAmount and PostingAmount
fields used when constructing the message) so the round-trip serialization test
fully verifies these values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7ecf1e2f-e061-4d94-954d-c76abc7006a9
📒 Files selected for processing (1)
tests/proto/serialization_test.go
bjcoombs
dismissed
coderabbitai[bot]’s stale review
Stale bot review - all threads resolved
The previous regex rejected valid positive values like "01" and "0001.50" that the base InstrumentAmount pattern accepts. Use a pattern that checks for any numeric string that is not all-zeros.
Proto validation requires version >= 1. Domain instruments created before versioning was introduced have version 0 as default.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
bjcoombs
deleted the
059-asset-agnostic-posting-layer--2-6--event-proto-migration
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
google.type.Moneyfields withmeridian.quantity.v1.InstrumentAmountinfinancial_accounting_events.proto(8 fields across 4 messages) andcurrent_account_events.proto(10 fields across 5 messages)amountfield: positive-amount regex for posting/transaction amounts, non-negative regex for totals/limits, no constraint for balances and variancegrpc_mappers.go(use existingtoProtoInstrumentAmount) andgrpc_control_endpoints.go(replacemoney.Moneyconstruction withInstrumentAmount)InstrumentAmountinstead ofMoneyContext
Part of the 059-asset-agnostic-posting-layer initiative (Tasks 2 and 6). Combined into a single PR because both tasks modify proto event files and share
buf generateoutput.Fields migrated in financial_accounting_events.proto
Fields migrated in current_account_events.proto
Test plan
go build ./...passes (excluding pre-existing clang linker crash on position-tool)