feat: topup wallet returns updated wallet data - #76
Conversation
Reviewer's GuideThis PR extends the TopupWallet flow to return the updated wallet object by adding a Data field in the response proto, updating service and repository methods to return the new wallet, adjusting SQL queries to RETURNING, and updating handlers and tests across wallet, auth, and user modules to reflect this change and fix end-to-end tests. Sequence diagram for Topup Wallet flow returning updated wallet datasequenceDiagram
participant Client
participant WalletCommandHandler
participant WalletTopupService
participant WalletRepository
participant DB
Client->>WalletCommandHandler: TopupWallet(request)
WalletCommandHandler->>WalletTopupService: Topup(req)
WalletTopupService->>WalletRepository: AddWalletBalance(walletID, amount)
WalletRepository->>DB: UPDATE wallets SET balance = balance + amount RETURNING *
DB-->>WalletRepository: Updated wallet row
WalletRepository-->>WalletTopupService: Updated Wallet entity
WalletTopupService-->>WalletCommandHandler: Updated Wallet entity
WalletCommandHandler-->>Client: TopupWalletResponse{Data: updated wallet}
ER diagram for updated TopupWalletResponse and Wallet entityerDiagram
TOPUP_WALLET_RESPONSE {
Wallet data
}
WALLET {
id UUID
user_id UUID
balance DECIMAL
created_at TIMESTAMP
updated_at TIMESTAMP
deleted_at TIMESTAMP
created_by UUID
updated_by UUID
deleted_by UUID
}
TOPUP_WALLET_RESPONSE ||--|| WALLET : contains
Class diagram for updated TopupWalletResponse and related service interfacesclassDiagram
class TopupWalletResponse {
+Wallet Data
+GetData()
}
class WalletCommand {
+TopupWallet(ctx, request) : TopupWalletResponse
}
class TopupWallet {
+Topup(ctx, topup) : Wallet
}
class TopupWalletRepository {
+AddWalletBalance(ctx, id, amount) : Wallet
}
TopupWalletResponse --> Wallet
WalletCommand --> TopupWallet
TopupWallet --> TopupWalletRepository
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
WalkthroughDocumentation comments across proto files are updated for consistency. Auth and user service tests are refactored to generate unique test credentials dynamically. The wallet service is extended: TopupWalletResponse now includes wallet data, and AddWalletBalance/Topup methods return the updated wallet object instead of only errors, requiring corresponding changes to database queries, service layers, and all related tests and mocks. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Handler as gRPC Handler
participant Service as Topup Service
participant Repo as Repository
participant DB as Database
rect rgb(200, 220, 240)
Note over Client,DB: Before: TopupWallet returned only error
Client->>Handler: TopupWallet()
Handler->>Service: Topup()
Service->>Repo: AddWalletBalance()
Repo->>DB: UPDATE + balance
DB-->>Repo: error
Repo-->>Service: error
Service-->>Handler: error
Handler-->>Client: empty TopupWalletResponse{}
end
rect rgb(220, 240, 200)
Note over Client,DB: After: TopupWallet returns wallet data
Client->>Handler: TopupWallet()
Handler->>Service: Topup()
Service->>Repo: AddWalletBalance()
Repo->>DB: UPDATE ... RETURNING *
DB-->>Repo: Wallet {ID, UserID, Balance}
Repo-->>Service: (*Wallet, error)
Service-->>Handler: (*Wallet, error)
Handler->>Handler: createWalletProto(wallet)
Handler-->>Client: TopupWalletResponse{Data: Wallet}
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)level=warning msg="[linters_context] running gomodguard failed: unable to read module file go.mod: current working directory must have a go.mod file: if you are not using go modules it is suggested to disable this linter" 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. Comment |
PR Compliance Guide 🔍(Compliance updated until commit 531dfe9)Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label Previous compliance checksCompliance check up to commit 531dfe9
|
||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Consider extending the updated wallet-return pattern to the TransferBalance API for consistent behavior across wallet operations.
- Ensure that adding the
datafield to TopupWalletResponse in the proto and Swagger spec does not break backward compatibility for existing clients. - Verify that the SQL
RETURNINGclause and accompanying--noqacomment in AddWalletBalance are correctly handled by your SQL codegen and do not introduce runtime errors.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider extending the updated wallet-return pattern to the TransferBalance API for consistent behavior across wallet operations.
- Ensure that adding the `data` field to TopupWalletResponse in the proto and Swagger spec does not break backward compatibility for existing clients.
- Verify that the SQL `RETURNING` clause and accompanying `--noqa` comment in AddWalletBalance are correctly handled by your SQL codegen and do not introduce runtime errors.
## Individual Comments
### Comment 1
<location> `service/wallet/internal/service/wallet_topup_test.go:131` </location>
<code_context>
+ wallet, err := st.topup.Topup(testCtx, topup)
assert.NoError(t, err)
+ assert.NotNil(t, wallet)
})
}
</code_context>
<issue_to_address>
**suggestion (testing):** Success test now asserts returned wallet is not nil.
Add assertions for wallet fields such as ID, UserID, and Balance to verify the returned data is correct.
Suggested implementation:
```golang
assert.NoError(t, err)
assert.NotNil(t, wallet)
assert.Equal(t, topup.WalletID, wallet.ID)
assert.Equal(t, topup.UserID, wallet.UserID)
assert.Equal(t, topup.Amount, wallet.Balance)
})
}
```
If `createTestWallet()` returns a wallet with fields other than those in `topup`, you may need to adjust the expected values in the assertions to match the actual test wallet's fields (e.g., use `expectedWallet := createTestWallet()` and compare to its fields).
</issue_to_address>
### Comment 2
<location> `service/wallet/internal/grpc/handler/wallet_command_test.go:190-193` </location>
<code_context>
+
st := createWalletCommandSuite(ctrl)
- st.topup.EXPECT().Topup(testCtxWithValidKey, gomock.Any()).Return(nil)
+ st.topup.EXPECT().Topup(testCtxWithValidKey, gomock.Any()).Return(&entity.Wallet{
+ ID: walletID,
+ UserID: userID,
+ Balance: decimal.NewFromFloat(10.23),
+ }, nil)
request := &apiv1.TopupWalletRequest{
</code_context>
<issue_to_address>
**suggestion (testing):** Success test now returns wallet entity from mock.
Add assertions to verify that the Data field in the response contains the expected wallet values.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #76 +/- ##
==========================================
- Coverage 61.22% 61.04% -0.19%
==========================================
Files 61 61
Lines 2094 2115 +21
==========================================
+ Hits 1282 1291 +9
- Misses 774 786 +12
Partials 38 38 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
||||||||||||
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
service/auth/test/integration/login_test.go (1)
17-22: Assert the registration succeeds before running the login casesLine 21 currently drops both the response and error from
RegisterAccount. If registration ever fails, the subsequent assertions will all report login failures, masking the real issue. Please capture the result and assert success so setup problems surface immediately.- _, _ = grpcClient.RegisterAccount(testCtxBasic, req) + res, err := grpcClient.RegisterAccount(testCtxBasic, req) + assert.NoError(t, err, "failed to register account for login test") + assert.NotEmpty(t, res)service/wallet/internal/service/wallet_transferer_test.go (1)
201-201: Align mocks with the new AddWalletBalance contract.Now that
AddWalletBalancereturns(*entity.Wallet, error), the happy-path expectations should hand back a non-nil wallet. Returningnilmasks regressions if the service later starts using the updated balances. Please switch the successful expectations to return a stub wallet (e.g.,createTestWallet()), keeping only the error paths atnil.Also applies to: 220-221, 240-241, 260-261
service/wallet/internal/repository/postgres/wallet.go (1)
54-66: Consider populating Auditable fields from the query result.The query returns
created_at,updated_at,created_by, andupdated_byfields (lines 31-41 in queries.sql.go), but they're not mapped to the returnedentity.Wallet. Sinceentity.WalletembedsAuditable, consider populating these fields for consistency with theGetUserWalletForUpdatepattern.Apply this diff if you want to include the audit fields:
return &entity.Wallet{ ID: res.ID, UserID: res.UserID, Balance: res.Balance, + Auditable: entity.Auditable{ + CreatedAt: res.CreatedAt, + UpdatedAt: res.UpdatedAt, + CreatedBy: res.CreatedBy, + UpdatedBy: res.UpdatedBy, + }, }, nil
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
go.work.sumis excluded by!**/*.sumproto/api/v1/auth_grpc.pb.gois excluded by!**/*.pb.goproto/api/v1/transaction_grpc.pb.gois excluded by!**/*.pb.goproto/api/v1/user_grpc.pb.gois excluded by!**/*.pb.goproto/api/v1/wallet.pb.gois excluded by!**/*.pb.goproto/api/v1/wallet_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (22)
openapiv2/arjuna.swagger.yaml(7 hunks)service/auth/api/v1/auth.proto(2 hunks)service/auth/test/integration/helper_test.go(0 hunks)service/auth/test/integration/login_test.go(4 hunks)service/auth/test/integration/register_test.go(2 hunks)service/transaction/api/v1/transaction.proto(1 hunks)service/user/api/v1/user.proto(3 hunks)service/user/test/integration/helper_test.go(0 hunks)service/user/test/integration/register_test.go(2 hunks)service/wallet/api/v1/wallet.proto(3 hunks)service/wallet/db/queries/queries.sql(1 hunks)service/wallet/internal/grpc/handler/wallet_command.go(2 hunks)service/wallet/internal/grpc/handler/wallet_command_test.go(3 hunks)service/wallet/internal/repository/db/queries.sql.go(2 hunks)service/wallet/internal/repository/postgres/wallet.go(1 hunks)service/wallet/internal/repository/postgres/wallet_test.go(2 hunks)service/wallet/internal/service/wallet_topup.go(2 hunks)service/wallet/internal/service/wallet_topup_test.go(4 hunks)service/wallet/internal/service/wallet_transferer.go(2 hunks)service/wallet/internal/service/wallet_transferer_test.go(4 hunks)service/wallet/test/mock/service/wallet_topup.go(2 hunks)service/wallet/test/mock/service/wallet_transferer.go(1 hunks)
💤 Files with no reviewable changes (2)
- service/user/test/integration/helper_test.go
- service/auth/test/integration/helper_test.go
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2024-09-28T10:58:40.061Z
Learnt from: indrasaputra
Repo: indrasaputra/arjuna PR: 56
File: service/user/internal/service/user_registrar_test.go:131-131
Timestamp: 2024-09-28T10:58:40.061Z
Learning: When mocking transactions in tests, using `context.WithValue` to pass transaction information is acceptable.
Applied to files:
service/wallet/internal/repository/postgres/wallet_test.goservice/wallet/test/mock/service/wallet_transferer.goservice/wallet/internal/service/wallet_transferer_test.goservice/wallet/internal/grpc/handler/wallet_command_test.goservice/wallet/test/mock/service/wallet_topup.go
📚 Learning: 2025-01-18T13:42:29.344Z
Learnt from: indrasaputra
Repo: indrasaputra/arjuna PR: 60
File: service/wallet/internal/service/wallet_transferer.go:84-93
Timestamp: 2025-01-18T13:42:29.344Z
Learning: When comparing UUIDs from github.com/google/uuid package, use bytes.Compare on the underlying byte slice (uuid[:]) instead of String() comparison to ensure correct ordering based on binary representation.
Applied to files:
service/wallet/internal/grpc/handler/wallet_command_test.goservice/auth/test/integration/register_test.go
📚 Learning: 2025-01-18T13:49:05.587Z
Learnt from: indrasaputra
Repo: indrasaputra/arjuna PR: 60
File: service/wallet/db/queries/queries.sql:8-9
Timestamp: 2025-01-18T13:49:05.587Z
Learning: In wallet-related SQL queries, while using just the ID is technically sufficient for identifying records, including user_id in WHERE clauses provides an additional layer of security to prevent unauthorized modifications.
Applied to files:
service/wallet/db/queries/queries.sql
🧬 Code graph analysis (11)
service/wallet/internal/repository/postgres/wallet.go (4)
service/wallet/internal/repository/db/models.go (1)
Wallet(14-24)service/wallet/entity/wallet.go (1)
Wallet(11-16)service/wallet/internal/repository/db/queries.sql.go (1)
AddWalletBalanceParams(22-25)service/wallet/entity/error.go (1)
ErrInternal(12-22)
service/wallet/test/mock/service/wallet_transferer.go (2)
service/wallet/internal/repository/db/models.go (1)
Wallet(14-24)service/wallet/entity/wallet.go (1)
Wallet(11-16)
service/wallet/internal/service/wallet_topup.go (2)
service/wallet/entity/wallet.go (2)
TopupWallet(19-24)Wallet(11-16)service/wallet/entity/error.go (1)
ErrEmptyWallet(38-53)
service/wallet/internal/service/wallet_topup_test.go (1)
service/wallet/entity/error.go (1)
ErrEmptyWallet(38-53)
service/wallet/internal/grpc/handler/wallet_command.go (2)
proto/api/v1/wallet.pb.go (9)
Topup(438-446)Topup(459-459)Topup(474-476)TopupWalletResponse(245-251)TopupWalletResponse(264-264)TopupWalletResponse(279-281)Wallet(374-384)Wallet(397-397)Wallet(412-414)service/wallet/entity/wallet.go (1)
Wallet(11-16)
service/wallet/internal/grpc/handler/wallet_command_test.go (2)
proto/api/v1/wallet.pb.go (6)
Topup(438-446)Topup(459-459)Topup(474-476)Wallet(374-384)Wallet(397-397)Wallet(412-414)service/wallet/entity/wallet.go (1)
Wallet(11-16)
service/wallet/internal/service/wallet_transferer.go (2)
service/wallet/internal/repository/db/models.go (1)
Wallet(14-24)service/wallet/entity/wallet.go (1)
Wallet(11-16)
service/auth/test/integration/register_test.go (1)
proto/api/v1/auth.pb.go (3)
Account(292-304)Account(317-317)Account(332-334)
service/auth/test/integration/login_test.go (1)
proto/api/v1/auth.pb.go (6)
RegisterAccountRequest(209-215)RegisterAccountRequest(228-228)RegisterAccountRequest(243-245)Account(292-304)Account(317-317)Account(332-334)
service/wallet/test/mock/service/wallet_topup.go (2)
service/wallet/internal/service/wallet_topup.go (1)
TopupWallet(14-18)service/wallet/entity/wallet.go (2)
TopupWallet(19-24)Wallet(11-16)
service/wallet/internal/repository/db/queries.sql.go (3)
service/wallet/internal/repository/db/db.go (1)
Queries(24-26)service/wallet/internal/repository/postgres/wallet.go (1)
Wallet(17-19)service/wallet/internal/repository/db/models.go (1)
Wallet(14-24)
🪛 Checkov (3.2.334)
openapiv2/arjuna.swagger.yaml
[medium] 34-58: Ensure that operation objects have 'consumes' field defined for PUT, POST and PATCH operations - version 2.0 files
(CKV_OPENAPI_17)
⏰ 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). (77)
- GitHub Check: Compile binary / Compile go binary
- GitHub Check: Compile binary / Compile go binary
- GitHub Check: Go code quality check / Semgrep scan
- GitHub Check: Go code quality check / Unit test and coverage
- GitHub Check: Go code quality check / go-code-lint
- GitHub Check: Go code quality check / Download go module
- GitHub Check: Sourcery review
- GitHub Check: Go code quality check / Unit test and coverage
- GitHub Check: Go code quality check / Unit test and coverage
- GitHub Check: Compile binary / Compile go binary
- GitHub Check: Compile binary / Compile go binary
- GitHub Check: Go code quality check / Semgrep scan
- GitHub Check: Go code quality check / Unit test and coverage
- GitHub Check: Go code quality check / go-code-lint
- GitHub Check: Go code quality check / Download go module
- GitHub Check: Sourcery review
- GitHub Check: Go code quality check / Unit test and coverage
- GitHub Check: Go code quality check / Unit test and coverage
- GitHub Check: Compile binary / Compile go binary
- GitHub Check: Compile binary / Compile go binary
- GitHub Check: Go code quality check / Semgrep scan
- GitHub Check: Go code quality check / Unit test and coverage
- GitHub Check: Go code quality check / go-code-lint
- GitHub Check: Go code quality check / Download go module
- GitHub Check: Sourcery review
- GitHub Check: Go code quality check / Unit test and coverage
- GitHub Check: Compile binary / Compile go binary
- GitHub Check: Compile binary / Compile go binary
- GitHub Check: Go code quality check / Semgrep scan
- GitHub Check: Go code quality check / Unit test and coverage
- GitHub Check: Go code quality check / go-code-lint
- GitHub Check: Sourcery review
- GitHub Check: Go code quality check / Unit test and coverage
- GitHub Check: Compile binary / Compile go binary
- GitHub Check: Compile binary / Compile go binary
- GitHub Check: Compile binary / Compile go binary
- GitHub Check: Go code quality check / Semgrep scan
- GitHub Check: Go code quality check / Unit test and coverage
- GitHub Check: Go code quality check / go-code-lint
- GitHub Check: Sourcery review
- GitHub Check: Go code quality check / Unit test and coverage
- GitHub Check: Compile binary / Compile go binary
- GitHub Check: Compile binary / Compile go binary
- GitHub Check: Compile binary / Compile go binary
- GitHub Check: Go code quality check / Semgrep scan
- GitHub Check: Go code quality check / Unit test and coverage
- GitHub Check: Go code quality check / go-code-lint
- GitHub Check: Sourcery review
- GitHub Check: Go code quality check / Unit test and coverage
- GitHub Check: Compile binary / Compile go binary
- GitHub Check: Compile binary / Compile go binary
- GitHub Check: Compile binary / Compile go binary
- GitHub Check: Go code quality check / Unit test and coverage
- GitHub Check: Go code quality check / go-code-lint
- GitHub Check: Sourcery review
- GitHub Check: Go code quality check / Unit test and coverage
- GitHub Check: Compile binary / Compile go binary
- GitHub Check: Compile binary / Compile go binary
- GitHub Check: Compile binary / Compile go binary
- GitHub Check: Go code quality check / Unit test and coverage
- GitHub Check: Go code quality check / go-code-lint
- GitHub Check: Sourcery review
- GitHub Check: Go code quality check / Unit test and coverage
- GitHub Check: Compile binary / Compile go binary
- GitHub Check: Compile binary / Compile go binary
- GitHub Check: Compile binary / Compile go binary
- GitHub Check: Go code quality check / Unit test and coverage
- GitHub Check: Go code quality check / go-code-lint
- GitHub Check: Sourcery review
- GitHub Check: Go code quality check / Unit test and coverage
- GitHub Check: Compile binary / Compile go binary
- GitHub Check: Compile binary / Compile go binary
- GitHub Check: Compile binary / Compile go binary
- GitHub Check: Go code quality check / Unit test and coverage
- GitHub Check: Go code quality check / go-code-lint
- GitHub Check: Sourcery review
- GitHub Check: Go code quality check / Unit test and coverage
🔇 Additional comments (18)
service/wallet/db/queries/queries.sql (1)
8-10: RETURNING clause looks solid.Switching
AddWalletBalanceto:onewithRETURNING *cleanly enables the repository to surface the updated wallet row; no issues spotted.service/wallet/api/v1/wallet.proto (1)
34-42: Proto updates match the new contract.Exposing
TopupWalletResponse.dataas anOUTPUT_ONLYwallet payload dovetails with the handler changes and keeps the schema consistent with swagger. Looks good.Also applies to: 68-96, 114-117
service/wallet/internal/grpc/handler/wallet_command.go (1)
73-79: Handler wiring cleanly surfaces the wallet payload.Capturing the wallet from
wc.topup.Topupand funnelling it throughcreateWalletProtois exactly what the new API surface needs; the helper keeps the conversion tidy.Also applies to: 125-130
openapiv2/arjuna.swagger.yaml (1)
358-363: Swagger definition stays in lockstep.Adding the
dataproperty referencingv1Walletkeeps the OpenAPI spec aligned with the proto/handler response change. Nicely synchronized.service/wallet/internal/repository/db/queries.sql.go (1)
16-43: LGTM! SQL query correctly updated to return wallet data.The change from
:execto:onewith aRETURNINGclause is appropriate for returning the updated wallet row. The scan operation correctly maps all returned columns to the Wallet struct fields.service/wallet/test/mock/service/wallet_topup.go (2)
48-54: LGTM! Mock updated correctly for new return signature.The mock method now correctly returns
(*entity.Wallet, error)to match the updated interface.
87-93: LGTM! AddWalletBalance mock updated correctly.The mock repository method signature and return handling are correct for the new
(*entity.Wallet, error)return type.service/wallet/internal/repository/postgres/wallet_test.go (2)
105-118: LGTM! Error test case correctly updated.The test now uses
ExpectQueryinstead ofExpectExecand properly asserts that the wallet isnilon error.
120-140: LGTM! Success test case validates returned wallet.The test correctly mocks the
RETURNINGclause with all wallet columns and validates the returned wallet's ID, UserID, and Balance fields.service/wallet/internal/grpc/handler/wallet_command_test.go (2)
159-183: LGTM! Error test cases updated correctly.The mock now returns
(nil, error)to match the new signature, and assertions correctly verify nil response.
185-206: LGTM! Success test case validates wallet return.The test correctly mocks a wallet return with proper ID, UserID, and Balance fields, and the request uses the same
walletIDfor consistency.service/wallet/internal/service/wallet_topup_test.go (4)
42-50: LGTM! Test correctly validates nil wallet on error.The test now captures the wallet return value and asserts it's nil when an error occurs.
52-72: LGTM! Error cases handle wallet return correctly.All error paths properly assert that the wallet is nil alongside the error.
110-120: LGTM! Repository error case updated correctly.The mock now returns
(nil, error)and the test validates nil wallet on error.
122-132: LGTM! Success case validates wallet return.The mock returns a populated wallet and the test correctly asserts the wallet is not nil on success.
service/wallet/test/mock/service/wallet_transferer.go (1)
86-92: LGTM! Mock correctly updated for new signature.The mock method now returns
(*entity.Wallet, error)with proper type assertions.service/wallet/internal/service/wallet_topup.go (2)
13-24: LGTM! Interfaces updated to return wallet data.The
TopupWalletandTopupWalletRepositoryinterfaces now correctly return(*entity.Wallet, error)to support returning the updated wallet.
45-66: LGTM! Implementation correctly returns wallet data.The
Topupmethod now returns the updated wallet on success and(nil, error)on all error paths, which is the correct pattern for this change.
Summary
topup wallet returns updated wallet data
Description
Summary by Sourcery
Return updated wallet data in the topup flow by extending the repository, service, handler, proto definitions, SQL queries, and tests, and fix related integration tests for auth flows.
New Features:
Enhancements:
Documentation:
Tests:
Summary by CodeRabbit
Release Notes
New Features
Documentation