feat(kyc): Implement wallet address migration for verified KYC records #515
Conversation
- Implement HandleWalletAddressUpdateForKYC function to handle wallet address updates for KYC records. - Validate incoming payload and ensure proper error handling for various scenarios. - Update the database with the new wallet address if all checks pass. - Add new route for the KYC wallet address update in the router configuration.
WalkthroughAdds a new KYC wallet address update flow: a PUT endpoint wired to a controller handler that validates a JSON payload, verifies a signature produced by the new wallet address, ensures the old address has a successful KYC, prevents reuse of an existing new address, updates the IdentityVerificationRequest with the new wallet address and timestamp, and returns the updated record or appropriate errors. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Router as Router (/v1/kyc/update_kyc_wallet_address)
participant Controller
participant Verifier as SignatureVerifier
participant KYC as IdentityVerificationRequest Store
Client->>Router: PUT {oldWalletAddress, newWalletAddress, signature, nonce}
Router->>Controller: HandleWalletAddressUpdateForKYC(ctx)
Controller->>Controller: Validate payload (non-empty, formats)
alt invalid payload
Controller-->>Client: 400 Bad Request
else valid
Controller->>Verifier: verify(newWalletAddress, signature, nonce)
alt signature invalid
Controller-->>Client: 401 / 400 (invalid signature)
else signature valid
Controller->>KYC: fetch by oldWalletAddress
alt not found
Controller-->>Client: 404 Not Found
else found
alt old address not StatusSuccess
Controller-->>Client: 400 Bad Request (KYC not verified)
else verified
Controller->>KYC: check existence of newWalletAddress
alt new address exists
Controller-->>Client: 409 / 400 (new address in use)
else available
Controller->>KYC: update record (walletAddress=new, updatedAt=now)
KYC-->>Controller: updated record
Controller-->>Client: 200 OK {updated details}
end
end
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
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: 5
🧹 Nitpick comments (4)
controllers/index.go (4)
2356-2367: Clarify intent: block any existing record for the new address; fix user-facing messageYou’re checking for existence regardless of status (good, per PR objective “not already associated”). The message says “already verified” which is misleading if a pending/failed record exists.
- if exists { - u.APIResponse(ctx, http.StatusBadRequest, "error", "New wallet address is already verified", nil) - return - } + if exists { + u.APIResponse(ctx, http.StatusBadRequest, "error", "New wallet address is already associated with a KYC record", nil) + return + }
2370-2379: Persist the new wallet signature for auditability and future checksYou already persist UpdatedAt and the new WalletAddress. Persisting the signature ties the migration action to on-chain ownership proof for the new address.
_, err = ivr. Update(). SetWalletAddress(payload.NewWalletAddress). + SetWalletSignature(payload.Signature). SetUpdatedAt(timestamp). Save(ctx)Optional: capture
payload.Nonceif you store/compare nonces for replay protection.
2356-2379: Race protection and uniquenessBetween the “exists” check and the update, a concurrent request could associate the same new address. If the DB enforces a unique index on
identity_verification_requests.wallet_address, you’re safe; otherwise, wrap this in a transaction and rely on the unique constraint to fail atomically.Sketch:
- Add a unique index on
wallet_address(if not already present).- In code, run the existence check and update in a transaction and handle unique constraint violations by returning a 400 with a friendly message.
2314-2386: Add audit logging for migrationsEmit a structured log (info) on both success and failure including old/new addresses, request origin, and correlation ID to satisfy audit requirements.
Example:
logger.WithFields(logger.Fields{ "oldWalletAddress": payload.OldWalletAddress, "newWalletAddress": payload.NewWalletAddress, "requestIP": ctx.ClientIP(), }).Info("KYC wallet address migration request processed")
📜 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 (3)
controllers/index.go(2 hunks)routers/index.go(1 hunks)types/types.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
controllers/index.go (8)
types/types.go (1)
UpdateKYCWalletAddressRequest(157-162)utils/http.go (2)
APIResponse(23-29)GetErrorData(55-64)ent/client.go (1)
Client(48-104)storage/database.go (1)
Client(21-21)ent/identityverificationrequest.go (2)
IdentityVerificationRequest(17-40)IdentityVerificationRequest(43-60)ent/ent.go (1)
IsNotFound(257-263)ent/identityverificationrequest/where.go (1)
WalletAddressEQ(94-96)ent/identityverificationrequest/identityverificationrequest.go (2)
Status(103-103)StatusSuccess(111-111)
🔇 Additional comments (1)
controllers/index.go (1)
24-24: Import looks correctThe new ent import for identityverificationrequest matches the usage below.
| func (ctrl *Controller) HandleWalletAddressUpdateForKYC(ctx *gin.Context) { | ||
| var payload types.UpdateKYCWalletAddressRequest | ||
|
|
||
| // Bind and validate JSON payload | ||
| if err := ctx.ShouldBindJSON(&payload); err != nil { | ||
| u.APIResponse(ctx, http.StatusBadRequest, "error", "Failed to validate payload", u.GetErrorData(err)) | ||
| return | ||
| } | ||
|
|
||
| // Basic payload validation | ||
| if payload.OldWalletAddress == "" || payload.NewWalletAddress == "" || payload.Signature == "" || payload.Nonce == "" { | ||
| u.APIResponse(ctx, http.StatusBadRequest, "error", "OldWalletAddress, NewWalletAddress, Signature, and Nonce are required", nil) | ||
| return | ||
| } | ||
|
|
||
| // Verify the signature signed by the new wallet address | ||
| if err := ctrl.verifyWalletSignature(payload.NewWalletAddress, payload.Signature, payload.Nonce); err != nil { | ||
| u.APIResponse(ctx, http.StatusUnauthorized, "error", "Invalid signature", err.Error()) | ||
| return | ||
| } |
There was a problem hiding this comment.
Missing server-side authorization; require an internal secret before any migration logic runs
Right now, possession of a valid signature from the new wallet is enough to trigger a migration. Without a server-side secret, an attacker who convinces someone to sign the generic “KYC policy” message could hijack a verified record by pairing it with a victim’s old (verified) address. This violates the “secret key validation” requirement and is a critical security issue.
Apply a server-held secret check (header-based) before signature verification. Adjust the config field to your actual one.
func (ctrl *Controller) HandleWalletAddressUpdateForKYC(ctx *gin.Context) {
var payload types.UpdateKYCWalletAddressRequest
// Bind and validate JSON payload
if err := ctx.ShouldBindJSON(&payload); err != nil {
u.APIResponse(ctx, http.StatusBadRequest, "error", "Failed to validate payload", u.GetErrorData(err))
return
}
+ // Server-side authorization: require internal secret to perform migrations
+ secret := ctx.GetHeader("x-kyc-update-secret")
+ if secret == "" || secret != identityConf.KYCMigrationSecret {
+ u.APIResponse(ctx, http.StatusUnauthorized, "error", "Unauthorized", nil)
+ return
+ }Notes:
- If you prefer middleware, move this logic into a reusable guard. Keep the handler small.
- Consider rate limiting and structured audit logs for this path.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (ctrl *Controller) HandleWalletAddressUpdateForKYC(ctx *gin.Context) { | |
| var payload types.UpdateKYCWalletAddressRequest | |
| // Bind and validate JSON payload | |
| if err := ctx.ShouldBindJSON(&payload); err != nil { | |
| u.APIResponse(ctx, http.StatusBadRequest, "error", "Failed to validate payload", u.GetErrorData(err)) | |
| return | |
| } | |
| // Basic payload validation | |
| if payload.OldWalletAddress == "" || payload.NewWalletAddress == "" || payload.Signature == "" || payload.Nonce == "" { | |
| u.APIResponse(ctx, http.StatusBadRequest, "error", "OldWalletAddress, NewWalletAddress, Signature, and Nonce are required", nil) | |
| return | |
| } | |
| // Verify the signature signed by the new wallet address | |
| if err := ctrl.verifyWalletSignature(payload.NewWalletAddress, payload.Signature, payload.Nonce); err != nil { | |
| u.APIResponse(ctx, http.StatusUnauthorized, "error", "Invalid signature", err.Error()) | |
| return | |
| } | |
| func (ctrl *Controller) HandleWalletAddressUpdateForKYC(ctx *gin.Context) { | |
| var payload types.UpdateKYCWalletAddressRequest | |
| // Bind and validate JSON payload | |
| if err := ctx.ShouldBindJSON(&payload); err != nil { | |
| u.APIResponse(ctx, http.StatusBadRequest, "error", "Failed to validate payload", u.GetErrorData(err)) | |
| return | |
| } | |
| // Server-side authorization: require internal secret to perform migrations | |
| secret := ctx.GetHeader("x-kyc-update-secret") | |
| if secret == "" || secret != identityConf.KYCMigrationSecret { | |
| u.APIResponse(ctx, http.StatusUnauthorized, "error", "Unauthorized", nil) | |
| return | |
| } | |
| // Basic payload validation | |
| if payload.OldWalletAddress == "" || payload.NewWalletAddress == "" || payload.Signature == "" || payload.Nonce == "" { | |
| u.APIResponse(ctx, http.StatusBadRequest, "error", "OldWalletAddress, NewWalletAddress, Signature, and Nonce are required", nil) | |
| return | |
| } | |
| // Verify the signature signed by the new wallet address | |
| if err := ctrl.verifyWalletSignature(payload.NewWalletAddress, payload.Signature, payload.Nonce); err != nil { | |
| u.APIResponse(ctx, http.StatusUnauthorized, "error", "Invalid signature", err.Error()) | |
| return | |
| } | |
| // ... | |
| } |
🤖 Prompt for AI Agents
In controllers/index.go around lines 2314 to 2333, add a server-side secret
check before any migration logic: read a configured internal secret value (e.g.
cfg.InternalWebhookSecret or the actual config field) and validate it against a
request header (e.g. "X-Internal-Secret"); if the header is missing or does not
match, respond with 401 and abort before performing signature verification or
any state changes. Implement the check at the top of this handler (or move into
reusable middleware/guard if preferred) so no further processing occurs without
the secret, and ensure the error response is logged/audited appropriately.
| // Basic payload validation | ||
| if payload.OldWalletAddress == "" || payload.NewWalletAddress == "" || payload.Signature == "" || payload.Nonce == "" { | ||
| u.APIResponse(ctx, http.StatusBadRequest, "error", "OldWalletAddress, NewWalletAddress, Signature, and Nonce are required", nil) | ||
| return | ||
| } | ||
|
|
||
| // Verify the signature signed by the new wallet address | ||
| if err := ctrl.verifyWalletSignature(payload.NewWalletAddress, payload.Signature, payload.Nonce); err != nil { | ||
| u.APIResponse(ctx, http.StatusUnauthorized, "error", "Invalid signature", err.Error()) | ||
| return | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add address validation, normalization, and Old==New guard in the handler
Even with binding rules, you should:
- Validate both addresses are proper hex EVM addresses.
- Normalize to EIP-55 checksummed format to avoid case-sensitivity mismatches with DB equality checks.
- Explicitly reject Old==New early with a clear error.
- // Basic payload validation
- if payload.OldWalletAddress == "" || payload.NewWalletAddress == "" || payload.Signature == "" || payload.Nonce == "" {
- u.APIResponse(ctx, http.StatusBadRequest, "error", "OldWalletAddress, NewWalletAddress, Signature, and Nonce are required", nil)
- return
- }
+ // Basic payload validation is also enforced via binding tags; add strict address checks
+ if !ethcommon.IsHexAddress(payload.OldWalletAddress) || !ethcommon.IsHexAddress(payload.NewWalletAddress) {
+ u.APIResponse(ctx, http.StatusBadRequest, "error", "Invalid wallet address format", nil)
+ return
+ }
+ // Normalize to checksummed addresses to ensure DB lookups are consistent
+ payload.OldWalletAddress = ethcommon.HexToAddress(payload.OldWalletAddress).Hex()
+ payload.NewWalletAddress = ethcommon.HexToAddress(payload.NewWalletAddress).Hex()
+ // Ensure the addresses are not the same
+ if strings.EqualFold(payload.OldWalletAddress, payload.NewWalletAddress) {
+ u.APIResponse(ctx, http.StatusBadRequest, "error", "Old and new wallet addresses cannot be the same", nil)
+ return
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Basic payload validation | |
| if payload.OldWalletAddress == "" || payload.NewWalletAddress == "" || payload.Signature == "" || payload.Nonce == "" { | |
| u.APIResponse(ctx, http.StatusBadRequest, "error", "OldWalletAddress, NewWalletAddress, Signature, and Nonce are required", nil) | |
| return | |
| } | |
| // Verify the signature signed by the new wallet address | |
| if err := ctrl.verifyWalletSignature(payload.NewWalletAddress, payload.Signature, payload.Nonce); err != nil { | |
| u.APIResponse(ctx, http.StatusUnauthorized, "error", "Invalid signature", err.Error()) | |
| return | |
| } | |
| // Basic payload validation is also enforced via binding tags; add strict address checks | |
| if !ethcommon.IsHexAddress(payload.OldWalletAddress) || !ethcommon.IsHexAddress(payload.NewWalletAddress) { | |
| u.APIResponse(ctx, http.StatusBadRequest, "error", "Invalid wallet address format", nil) | |
| return | |
| } | |
| // Normalize to checksummed addresses to ensure DB lookups are consistent | |
| payload.OldWalletAddress = ethcommon.HexToAddress(payload.OldWalletAddress).Hex() | |
| payload.NewWalletAddress = ethcommon.HexToAddress(payload.NewWalletAddress).Hex() | |
| // Ensure the addresses are not the same | |
| if strings.EqualFold(payload.OldWalletAddress, payload.NewWalletAddress) { | |
| u.APIResponse(ctx, http.StatusBadRequest, "error", "Old and new wallet addresses cannot be the same", nil) | |
| return | |
| } | |
| // Verify the signature signed by the new wallet address | |
| if err := ctrl.verifyWalletSignature(payload.NewWalletAddress, payload.Signature, payload.Nonce); err != nil { | |
| u.APIResponse(ctx, http.StatusUnauthorized, "error", "Invalid signature", err.Error()) | |
| return | |
| } |
🤖 Prompt for AI Agents
In controllers/index.go around lines 2323 to 2333, add explicit address
validation, normalization and an Old==New guard before signature verification:
use a reliable EVM address checker (e.g. common.IsHexAddress) to reject non-hex
EVM addresses with a 400 response; normalize both payload.OldWalletAddress and
payload.NewWalletAddress to the EIP-55 checksummed form (e.g.
common.HexToAddress(...).Hex()) and use the normalized values for all subsequent
checks; after normalization, if old == new return a 400 with a clear message
rejecting identical addresses; then proceed to call verifyWalletSignature using
the normalized new address. Ensure error messages and returned values use the
normalized addresses so DB equality and signature checks are consistent.
| // Verify the signature signed by the new wallet address | ||
| if err := ctrl.verifyWalletSignature(payload.NewWalletAddress, payload.Signature, payload.Nonce); err != nil { | ||
| u.APIResponse(ctx, http.StatusUnauthorized, "error", "Invalid signature", err.Error()) | ||
| return | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Signature must bind to the specific “migration” operation (domain separation) and ideally include both addresses
The current verification reuses the generic message (“I accept the KYC Policy…”) tied only to the wallet and nonce. This enables cross-intent replay: any prior signature for KYC could be accepted here. At minimum, the message must be specific to “migrate KYC from {old} to {new} with nonce {n}”.
Follow-up refactor (illustrative helper; place alongside verifyWalletSignature):
// verifyWalletSignatureForMigration verifies a signature over a purpose-built message.
func (ctrl *Controller) verifyWalletSignatureForMigration(newAddr, oldAddr, signature, nonce string) error {
sig, err := hex.DecodeString(strings.TrimPrefix(signature, "0x"))
if err != nil || len(sig) != 65 { return fmt.Errorf("invalid signature") }
if sig[64] != 27 && sig[64] != 28 { return fmt.Errorf("invalid signature") }
sig[64] -= 27
// Operation-specific message (domain separation)
message := fmt.Sprintf("I authorize migrating my KYC record from %s to %s with nonce %s", oldAddr, newAddr, nonce)
prefix := "\x19Ethereum Signed Message:\n" + fmt.Sprint(len(message))
hash := crypto.Keccak256Hash([]byte(prefix + message))
pk, err := crypto.SigToPub(hash.Bytes(), sig)
if err != nil { return fmt.Errorf("invalid signature") }
recovered := crypto.PubkeyToAddress(*pk).Hex()
if !strings.EqualFold(recovered, newAddr) { return fmt.Errorf("invalid signature") }
return nil
}Then use it here:
- if err := ctrl.verifyWalletSignature(payload.NewWalletAddress, payload.Signature, payload.Nonce); err != nil {
+ if err := ctrl.verifyWalletSignatureForMigration(payload.NewWalletAddress, payload.OldWalletAddress, payload.Signature, payload.Nonce); err != nil {
u.APIResponse(ctx, http.StatusUnauthorized, "error", "Invalid signature", err.Error())
return
}Additionally:
- Introduce a nonce store (single-use with TTL) to prevent replay across requests.
- Consider dual consent: require a signature from the OLD address too, unless the server-side secret gate is strictly controlled and audited.
🤖 Prompt for AI Agents
controllers/index.go around lines 2329 to 2333: the signature verification call
must be replaced with an operation-specific verification that binds the intent
to migrate KYC between the two addresses and nonce; implement a new helper
(e.g., verifyWalletSignatureForMigration(newAddr, oldAddr, signature, nonce))
that decodes and normalizes the signature, enforces v value, constructs the
domain-separated message "I authorize migrating my KYC record from {old} to
{new} with nonce {n}", prefixes it with the Ethereum signed-message header,
hashes and recovers the signer, and compares the recovered address to the
expected signer (use the new address if expecting new-signer or both addresses
if you require dual consent), then replace the current verifyWalletSignature
call with verifyWalletSignatureForMigration(payload.NewWalletAddress,
payload.OldWalletAddress, payload.Signature, payload.Nonce); additionally
integrate a single-use TTL nonce store to mark nonces as consumed and consider
optionally requiring a signature from the old address for dual consent.
| v1.POST("kyc", ctrl.RequestIDVerification) | ||
| v1.GET("kyc/:wallet_address", ctrl.GetIDVerificationStatus) | ||
| v1.POST("kyc/webhook", ctrl.KYCWebhook) | ||
| v1.PUT("kyc/update_kyc_wallet_address", ctrl.HandleWalletAddressUpdateForKYC) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Route is unauthenticated; add a server-side secret/auth gate as per PR objectives
The PR objective says “secret key validation for authorized access,” but the route is exposed with no middleware. Anyone who can craft a valid signature from the new wallet can attempt to migrate any verified record, which is not acceptable.
Action:
- Protect this endpoint with a server-controlled secret middleware (e.g., X-KYC-Update-Secret) or an internal-only network gate.
- Optionally restrict by IP allowlist (ops) and add request logging/metrics.
Example (adjust to your actual middleware/config):
- v1.PUT("kyc/update_kyc_wallet_address", ctrl.HandleWalletAddressUpdateForKYC)
+ v1.PUT(
+ "kyc/update_kyc_wallet_address",
+ // TODO: replace with your actual secret/auth middleware
+ middleware.KYCMigrationSecretMiddleware,
+ ctrl.HandleWalletAddressUpdateForKYC,
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| v1.PUT("kyc/update_kyc_wallet_address", ctrl.HandleWalletAddressUpdateForKYC) | |
| v1.PUT( | |
| "kyc/update_kyc_wallet_address", | |
| // TODO: replace with your actual secret/auth middleware | |
| middleware.KYCMigrationSecretMiddleware, | |
| ctrl.HandleWalletAddressUpdateForKYC, | |
| ) |
🤖 Prompt for AI Agents
In routers/index.go around line 60 the route
v1.PUT("kyc/update_kyc_wallet_address", ...) is unauthenticated; add a
server-side secret gate by creating/using middleware that reads a configured
secret header (e.g., X-KYC-Update-Secret) and rejects requests with 401/403 when
the header is missing or does not match the server-stored value, then apply that
middleware to this specific route registration; optionally also add an IP
allowlist check (ops-controlled) inside the same middleware and emit structured
request logs/metrics for accepted/rejected attempts.
| // UpdateKYCWalletAddressRequest represents a request to move a KYC record to a new wallet address | ||
| type UpdateKYCWalletAddressRequest struct { | ||
| OldWalletAddress string `json:"oldWalletAddress"` | ||
| NewWalletAddress string `json:"newWalletAddress"` | ||
| Signature string `json:"signature"` | ||
| Nonce string `json:"nonce"` | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make payload self-validating with binding rules and prevent Old==New at the schema level
Add binding tags so Gin catches bad/empty input early and ensure the new address differs from the old one. This reduces controller branching and standardizes error formatting.
type UpdateKYCWalletAddressRequest struct {
- OldWalletAddress string `json:"oldWalletAddress"`
- NewWalletAddress string `json:"newWalletAddress"`
- Signature string `json:"signature"`
- Nonce string `json:"nonce"`
+ OldWalletAddress string `json:"oldWalletAddress" binding:"required"`
+ NewWalletAddress string `json:"newWalletAddress" binding:"required,nefield=OldWalletAddress"`
+ Signature string `json:"signature" binding:"required"`
+ Nonce string `json:"nonce" binding:"required"`
}Follow-up:
- If you have a custom validator, consider
eth_addr(checksumming/length) for both addresses to front-load address validation.
- Implement tests for the new HandleWalletAddressUpdateForKYC endpoint. - Cover various scenarios including successful updates, invalid signatures, and missing fields. - Ensure proper database updates and response validation for KYC wallet address changes.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
controllers/index_test.go (2)
751-784: Consider extracting test cleanup to defer statement.The cleanup at line 782-783 could be moved to a defer statement immediately after creating the test record to ensure cleanup happens even if the test fails.
// Create a pending KYC record pendingIVR, err := db.Client.IdentityVerificationRequest. Create(). SetWalletAddress("0x8888888888888888888888888888888888888888"). SetWalletSignature("pending-signature"). SetPlatform(identityverificationrequest.PlatformSmileID). SetStatus(identityverificationrequest.StatusPending). SetVerificationURL("https://example.com/verify"). SetPlatformRef("test-ref-pending"). Save(context.Background()) assert.NoError(t, err) +defer func() { + _ = db.Client.IdentityVerificationRequest.DeleteOne(pendingIVR).Exec(context.Background()) +}() payload := types.UpdateKYCWalletAddressRequest{ OldWalletAddress: "0x8888888888888888888888888888888888888888", NewWalletAddress: newWalletAddress, Signature: signatureHex, Nonce: nonce, } res, err := test.PerformRequest(t, "PUT", "/kyc/update_kyc_wallet_address", payload, nil, router) assert.NoError(t, err) assert.Equal(t, http.StatusBadRequest, res.Code) var response types.Response err = json.Unmarshal(res.Body.Bytes(), &response) assert.NoError(t, err) assert.Equal(t, "error", response.Status) assert.Equal(t, "This account has not been verified", response.Message) - -// Clean up -err = db.Client.IdentityVerificationRequest.DeleteOne(pendingIVR).Exec(context.Background()) -assert.NoError(t, err)
582-583: Consider using deterministic test data.Using
time.Now().UnixNano()for generating test wallet addresses (line 582) and nonces (line 583) could potentially cause issues in parallel test execution or when debugging. Consider using fixed test data or a test-specific seed.-timestamp := time.Now().UnixNano() -oldWalletAddress := fmt.Sprintf("0xf4c5c4deDde7A86b25E7430796441e209e23eBF%d", timestamp%1000) -nonce := fmt.Sprintf("test-nonce-%d", timestamp) +// Use a unique but deterministic identifier for this test suite +testID := "kyc-wallet-update-test" +oldWalletAddress := fmt.Sprintf("0xf4c5c4deDde7A86b25E7430796441e209e23EB%s", "F1") +nonce := fmt.Sprintf("test-nonce-%s", testID)
📜 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 (1)
controllers/index_test.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
controllers/index_test.go (7)
ent/client.go (1)
Client(48-104)storage/database.go (1)
Client(21-21)ent/identityverificationrequest/identityverificationrequest.go (1)
PlatformSmileID(82-82)types/types.go (2)
UpdateKYCWalletAddressRequest(157-162)Response(566-570)utils/test/test.go (1)
PerformRequest(14-25)ent/ent.go (1)
IsNotFound(257-263)ent/identityverificationrequest/where.go (2)
WalletAddressEQ(94-96)WalletAddress(59-61)
⏰ 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: build-and-test
🔇 Additional comments (3)
controllers/index_test.go (3)
579-785: Well-structured comprehensive test suite for KYC wallet address update.The test suite thoroughly covers the new
HandleWalletAddressUpdateForKYCendpoint with appropriate test cases including successful migration, validation failures, and edge cases. The implementation correctly uses Ethereum-style signature verification and maintains data integrity.
586-614: Robust signature generation and verification setup.Good implementation of the Ethereum signature generation for testing. The code properly generates a private key, derives the wallet address, creates a proper EIP-191 formatted message, and adjusts the recovery ID to match Ethereum's expected format.
628-670: Comprehensive validation of successful wallet address update.The test properly validates:
- HTTP status and response structure
- Database state after update (new address exists with verified status)
- Old address removal from database
- Response payload completeness
Description
This PR introduces the UpdateKYCWalletAddress function to the SmileID KYC service.
The new function allows for securely migrating a verified KYC record from one wallet address to another, with appropriate validation and error handling.
Which includes:
Testing
This PR is covered by new unit tests in services/kyc/smile/index_test.go under the UpdateKYCWalletAddress test suite.
Test cases include:
Checklist
mainBy submitting a PR, I agree to Paycrest's Contributor Code of Conduct and Contribution Guide.
Summary by CodeRabbit
New Features
Tests