Conversation
WalkthroughUpdates CI to trigger only on pull_request to main/dev. Adds package Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant GH as GitHub
participant CI as CI Workflow
Dev->>GH: Open/Update Pull Request against main/dev
GH-->>CI: pull_request event
CI->>CI: Run CI jobs
Dev->>GH: Push to main/dev
GH--x CI: push event ignored (no trigger)
Note over GH,CI: CI now triggers only on pull_request for main/dev
sequenceDiagram
autonumber
participant Call as Caller
participant EZ as ezutil
participant DEC as decimal lib
participant PB as protobuf/money
Call->>EZ: FromProtoTime(timestamppb)
EZ->>PB: timestamppb.AsTime() / handle nil
EZ-->>Call: time.Time
Call->>EZ: DecimalToMoney(decimal, "USD")
EZ->>DEC: round/scale to 9 decimals
EZ->>EZ: compute units and nanos, normalize sign/range
EZ->>PB: construct & return `money.Money`
Call->>EZ: MoneyToDecimal(money.Money)
EZ->>PB: read units+nanos
EZ->>DEC: compose decimal from units + nanos/1e9
EZ-->>Call: decimal.Decimal
Call->>EZ: ValidateMoney(money.Money)
EZ->>EZ: check nil, nanos range, sign consistency
EZ-->>Call: error | nil
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
✨ 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 (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| return &money.Money{ | ||
| CurrencyCode: currencyCode, | ||
| Units: units, | ||
| Nanos: int32(nanos), |
Check failure
Code scanning / gosec
integer overflow conversion int64 -> int32 Error
| return &money.Money{ | ||
| CurrencyCode: currencyCode, | ||
| Units: units, | ||
| Nanos: int32(nanos), |
Check failure
Code scanning / gosec
integer overflow conversion int64 -> int32 Error
| return &money.Money{ | ||
| CurrencyCode: currencyCode, | ||
| Units: units, | ||
| Nanos: int32(nanos), |
Check failure
Code scanning / gosec
integer overflow conversion int64 -> int32 Error
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
go.mod (1)
3-3: Invalid go directive format; drop patch version.The go directive accepts major.minor only. Use “go 1.25”.
-go 1.25.0 +go 1.25
🧹 Nitpick comments (6)
generic_mappers.go (3)
12-17: Avoid potential panic: validate protobuf Timestamp before AsTime().timestamppb.Timestamp.AsTime() can panic on invalid values. Cheap guard keeps this helper safe.
func FromProtoTime(t *timestamppb.Timestamp) time.Time { if t == nil { return time.Time{} } - return t.AsTime() + // Ensure the protobuf timestamp is valid to avoid panics. + if err := t.CheckValid(); err != nil { + return time.Time{} + } + return t.AsTime() }
49-55: Be explicit about rounding mode (banker’s vs half-up).shopspring/decimal supports multiple rounding modes; codify the intended one to prevent regressions across environments.
If you want banker’s rounding:
- rounded := d.Round(9) + rounded := d.RoundBank(9)Otherwise, add a comment stating that half-up is intended.
57-74: Consider minimal extra validation (currency code).Optionally reject empty CurrencyCode to catch bad inputs early.
func ValidateMoney(m *money.Money) error { if m == nil { return eris.New("money cannot be nil") } + if m.CurrencyCode == "" { + return eris.New("currency code cannot be empty") + }If you add this, import "strings" only if you plan to trim whitespace as well.
test/generic_mappers_test.go (3)
20-25: Make time equality robust against monotonic clock fields.Comparing Unix seconds and nanoseconds avoids flakiness from monotonic components.
- result := ezutil.FromProtoTime(proto) - assert.True(t, now.Equal(result)) + result := ezutil.FromProtoTime(proto) + assert.Equal(t, now.Unix(), result.Unix()) + assert.Equal(t, now.Nanosecond(), result.Nanosecond())
51-56: Prefer NewFromString over NewFromFloat in tests.Avoid float parsing ambiguity; strings are exact and make tests future-proof.
- decimal: decimal.NewFromFloat(123.45), + decimal: decimal.RequireFromString("123.45"), ... - decimal: decimal.NewFromFloat(-50.25), + decimal: decimal.RequireFromString("-50.25"), ... - expected: decimal.NewFromFloat(123.45), + expected: decimal.RequireFromString("123.45"),Also applies to: 58-63, 112-113
133-151: Add a negative rounding case.Covers symmetry and sign handling for DecimalToMoneyRounded.
func TestDecimalToMoneyRounded(t *testing.T) { @@ t.Run("already rounded decimal", func(t *testing.T) { d := decimal.NewFromFloat(100.5) result := ezutil.DecimalToMoneyRounded(d, "EUR") @@ assert.Equal(t, int32(500000000), result.Nanos) }) + + t.Run("negative high precision decimal", func(t *testing.T) { + d := decimal.RequireFromString("-123.123456789123456") + result := ezutil.DecimalToMoneyRounded(d, "USD") + assert.Equal(t, "USD", result.CurrencyCode) + assert.Equal(t, int64(-123), result.Units) + assert.Equal(t, int32(-123456789), result.Nanos) + }) }
📜 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 (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
.github/workflows/ci.yml(0 hunks)generic_mappers.go(1 hunks)go.mod(1 hunks)test/generic_mappers_test.go(1 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/ci.yml
🧰 Additional context used
🧬 Code graph analysis (1)
test/generic_mappers_test.go (1)
generic_mappers.go (5)
FromProtoTime(12-17)DecimalToMoney(19-32)MoneyToDecimal(35-47)DecimalToMoneyRounded(51-55)ValidateMoney(58-74)
🪛 GitHub Check: gosec
generic_mappers.go
[failure] 30-30: integer overflow conversion int64 -> int32
integer overflow conversion int64 -> int32
[failure] 30-30: integer overflow conversion int64 -> int32
integer overflow conversion int64 -> int32
[failure] 30-30: integer overflow conversion int64 -> int32
integer overflow conversion int64 -> int32
🔇 Additional comments (3)
generic_mappers.go (1)
34-47: MoneyToDecimal: LGTM.Nil-safe and precise conversion using decimal scaling.
go.mod (1)
10-11: genproto@v0.0.0-20250826171959-ef028d996bc1 and protobuf@v1.36.8 resolve cleanly withgo mod tidyand passgo vet.test/generic_mappers_test.go (1)
153-233: Validation tests: LGTM.Good coverage of nil, bounds, and sign-mismatch scenarios.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
generic_mappers.go (1)
37-51: Quell gosec G115 and make the int64→int32 cast provably safe.Static analysis flags the cast at Line 50. Guard the bounds, cast to a local int32, and annotate the safe conversion. Also prefer preparing nanos32 after normalization rather than clamping silently in-place.
Apply:
- // Validate nanos bounds and int32 range - if nanos < -999_999_999 || nanos > 999_999_999 || nanos < int64(int32(-2147483648)) || nanos > int64(int32(2147483647)) { - // Clamp to valid range - if nanos > 999_999_999 { - nanos = 999_999_999 - } else if nanos < -999_999_999 { - nanos = -999_999_999 - } - } - - return &money.Money{ - CurrencyCode: currencyCode, - Units: units, - Nanos: int32(nanos), - } + // Prepare safe int32 cast (normalized nanos must be within [-999_999_999, 999_999_999]) + var nanos32 int32 + switch { + case nanos < -999_999_999: + nanos32 = -999_999_999 + case nanos > 999_999_999: + nanos32 = 999_999_999 + default: + // safe after normalization; explicit guard satisfies analyzers + nanos32 = int32(nanos) // #nosec G115 + } + + return &money.Money{ + CurrencyCode: currencyCode, + Units: units, + Nanos: nanos32, + }Optionally, replace magic 1_000_000_000 with a module-level const for clarity.
🧹 Nitpick comments (4)
generic_mappers.go (4)
20-23: Make DecimalToMoney non-rounding; keep rounding only in DecimalToMoneyRounded.Avoid double rounding and keep DecimalToMoney a truncating mapper; reserve rounding for DecimalToMoneyRounded. This also aligns with typical "exact split" mappers.
Apply:
- nanos := fractional.Mul(decimal.New(1000000000, 0)).Round(0).IntPart() + nanos := fractional.Mul(decimal.New(1_000_000_000, 0)).Truncate(0).IntPart()Please confirm intended rounding semantics for negative half-way cases.
78-94: Optional: validate currency code shape.If desired, enforce ISO-like 3-letter codes (when non-empty).
Apply:
func ValidateMoney(m *money.Money) error { if m == nil { return eris.New("money cannot be nil") } + if m.CurrencyCode != "" && len(m.CurrencyCode) != 3 { + return eris.Errorf("currency code must be 3 letters: %q", m.CurrencyCode) + }
12-17: Nit: add GoDoc comments for exported helpers.Improves discoverability and passes linters.
Example:
-func FromProtoTime(t *timestamppb.Timestamp) time.Time { +// FromProtoTime returns t.AsTime() or the zero time when t is nil. +func FromProtoTime(t *timestamppb.Timestamp) time.Time {Also applies to: 19-52, 55-67, 71-75, 78-94
19-35: Nit: factor nanos-per-unit constant.Removes magic numbers and keeps arithmetic consistent.
Apply:
+const nanosPerUnit int64 = 1_000_000_000 ... - nanos := fractional.Mul(decimal.New(1_000_000_000, 0)).Truncate(0).IntPart() + nanos := fractional.Mul(decimal.New(nanosPerUnit, 0)).Truncate(0).IntPart() ... - units += nanos / 1_000_000_000 - nanos = nanos % 1_000_000_000 + units += nanos / nanosPerUnit + nanos = nanos % nanosPerUnit ... - nanos += 1_000_000_000 + nanos += nanosPerUnit ... - nanos -= 1_000_000_000 + nanos -= nanosPerUnit
📜 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 selected for processing (2)
generic_mappers.go(1 hunks)test/generic_mappers_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/generic_mappers_test.go
🧰 Additional context used
🪛 GitHub Check: gosec
generic_mappers.go
[failure] 50-50: integer overflow conversion int64 -> int32
integer overflow conversion int64 -> int32
[failure] 50-50: integer overflow conversion int64 -> int32
integer overflow conversion int64 -> int32
[failure] 50-50: integer overflow conversion int64 -> int32
integer overflow conversion int64 -> int32
🔇 Additional comments (5)
generic_mappers.go (5)
12-17: LGTM: simple, nil-safe time conversion.
24-35: Normalization and sign alignment look correct.Folds whole billions and enforces consistent sign between units/nanos as expected.
55-67: LGTM: Money→Decimal conversion is precise and nil-safe.
71-75: LGTM: delegated rounding API is clean.After making DecimalToMoney truncating, this function remains the single rounding entrypoint.
37-51: Verification complete
Static analysis shows no G115 violation at generic_mappers.go:50 and the Money/ProtoTime tests pass.
2 participants
Summary by CodeRabbit
Chores
New Features
Tests