fix!: replaced sdkerrors.Register with sdkerrors.RegisterWithGRPCCode for the proper error propagation. #173
Conversation
WalkthroughReplace many package error initializers with gRPC-aware registrations by switching to Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code graph analysis (4)go/node/audit/v1/errors.go (1)
go/node/cert/v1/errors.go (1)
go/node/market/v1/errors.go (1)
go/node/deployment/v1/errors.go (1)
⏰ 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). (4)
🔇 Additional comments (5)
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 |
![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
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
go/node/deployment/v1/errors.gogo/node/deployment/v1/errors_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
go/node/deployment/v1/errors.go (1)
go/node/types/attributes/v1/attribute.go (1)
ModuleName(17-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). (3)
- GitHub Check: coverage
- GitHub Check: test
- GitHub Check: go
🔇 Additional comments (5)
go/node/deployment/v1/errors.go (3)
5-5: LGTM! Correct import for gRPC codes.The addition of the
google.golang.org/grpc/codesimport is necessary for the newRegisterWithGRPCCodecalls and enables proper HTTP status code mapping.
36-42: Excellent fix for the core issue!The mapping of
ErrDeploymentNotFoundtocodes.NotFound(line 42) directly addresses issue #404 by ensuring that missing deployment queries return HTTP 404 instead of HTTP 500. The gRPC code mappings for related "not found" errors are also correct.
44-81: LGTM! All remaining gRPC code mappings are semantically correct.The error-to-status-code mappings are appropriate:
FailedPreconditionfor state-based errors (closed, open, paused deployments/groups)InvalidArgumentfor all validation failuresInternalfor internal server errorsThis will ensure correct HTTP status codes are returned via grpc-gateway.
go/node/deployment/v1/errors_test.go (2)
1-11: LGTM! Proper test structure and imports.Using an external test package (
v1_test) is good practice for testing public APIs, and the imports are correct for validating gRPC status code mappings.
13-143: Excellent test coverage and implementation!The test provides complete coverage of all 23 error definitions with correct validation approach:
- Uses
status.FromErrorto properly extract gRPC status codes- Verifies both conversion success and code correctness
- Table-driven structure with descriptive test case names
- Covers all gRPC code categories: NotFound (3), AlreadyExists (1), InvalidArgument (13), FailedPrecondition (5), and Internal (1)
This ensures the fix for issue #404 is properly validated and prevents regressions.
…to propagate the proper http code into the response
vertex451
force-pushed
the
artem/fix/500-instead-of-404
branch
from
e970797 to
dc80e0d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
go/node/deployment/v1/errors_test.go (1)
13-13: Consider adding a package-level doc comment.A brief comment explaining that this test ensures proper HTTP status codes via grpc-gateway mapping would help future maintainers understand the test's purpose in the context of the HTTP 500 → 404 fix.
🔎 Suggested addition
+// Package v1_test validates gRPC status code mappings for deployment errors. +// These mappings ensure grpc-gateway returns correct HTTP status codes +// (e.g., 404 for NotFound instead of 500 for Unknown). + package v1_test
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
go/node/deployment/v1/errors.gogo/node/deployment/v1/errors_test.go
🧰 Additional context used
🧬 Code graph analysis (2)
go/node/deployment/v1/errors_test.go (1)
go/node/deployment/v1/errors.go (23)
ErrDeploymentNotFound(42-42)ErrGroupNotFound(62-62)ErrNameDoesNotExist(36-36)ErrDeploymentExists(40-40)ErrInvalidRequest(38-38)ErrInvalidGroups(48-48)ErrInvalidDeploymentID(50-50)ErrEmptyHash(52-52)ErrInvalidHash(54-54)ErrInvalidDeployment(58-58)ErrInvalidGroupID(60-60)ErrInvalidDeposit(74-74)ErrInvalidIDPath(76-76)ErrInvalidParam(78-78)ErrInvalidEscrowID(80-80)ErrOwnerAcctMissing(46-46)ErrGroupSpecInvalid(72-72)ErrDeploymentClosed(44-44)ErrGroupClosed(64-64)ErrGroupOpen(66-66)ErrGroupPaused(68-68)ErrGroupNotOpen(70-70)ErrInternal(56-56)
go/node/deployment/v1/errors.go (1)
go/node/types/attributes/v1/attribute.go (1)
ModuleName(17-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). (4)
- GitHub Check: test
- GitHub Check: coverage
- GitHub Check: go
- GitHub Check: proto
🔇 Additional comments (2)
go/node/deployment/v1/errors.go (1)
5-5: LGTM! Error-to-HTTP-status mappings are correct.The addition of explicit gRPC codes to all error registrations directly addresses the PR objective. The NotFound codes (lines 36, 42, 62) will now correctly map to HTTP 404 instead of 500 via grpc-gateway, fixing the reported issue. All other code mappings are semantically appropriate:
AlreadyExists→ HTTP 409InvalidArgument→ HTTP 400FailedPrecondition→ HTTP 400Internal→ HTTP 500Also applies to: 36-80
go/node/deployment/v1/errors_test.go (1)
13-143: LGTM! Comprehensive test coverage for gRPC code mappings.The test validates all 23 error variables against their expected gRPC codes using
status.FromError, which confirms the error registrations will produce the correct HTTP status codes via grpc-gateway. Test structure is clean and follows Go's table-driven testing pattern.
…in audit, cert, escrow, market, provider, take and types modules
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
go/node/market/v1/errors.go (1)
111-112: Add documentation comment forErrInvalidEscrowID.All other errors in this file have documentation comments. For consistency, add one for the new error.
🔎 Suggested fix
// ErrInvalidLeaseClosedReason indicates reason for lease close does not match context ErrInvalidLeaseClosedReason = sdkerrors.RegisterWithGRPCCode(ModuleName, errInvalidLeaseClosedReason, codes.InvalidArgument, "invalid lease closed reason") - ErrInvalidEscrowID = sdkerrors.RegisterWithGRPCCode(ModuleName, errInvalidEscrowID, codes.InvalidArgument, "invalid escrow id") + // ErrInvalidEscrowID indicates an invalid escrow ID + ErrInvalidEscrowID = sdkerrors.RegisterWithGRPCCode(ModuleName, errInvalidEscrowID, codes.InvalidArgument, "invalid escrow id")
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
go/node/audit/v1/errors.gogo/node/audit/v1/errors_test.gogo/node/cert/v1/errors.gogo/node/cert/v1/errors_test.gogo/node/deployment/v1/errors.gogo/node/escrow/module/error.gogo/node/escrow/module/error_test.gogo/node/market/v1/errors.gogo/node/market/v1/errors_test.gogo/node/provider/v1beta4/errors.gogo/node/provider/v1beta4/errors_test.gogo/node/take/v1/errors.gogo/node/take/v1/errors_test.gogo/node/types/attributes/v1/attribute.gogo/node/types/attributes/v1/errors_test.gogo/node/types/deposit/v1/errors.gogo/node/types/deposit/v1/errors_test.go
🧰 Additional context used
🧬 Code graph analysis (16)
go/node/take/v1/errors.go (1)
go/node/types/attributes/v1/attribute.go (1)
ModuleName(18-18)
go/node/audit/v1/errors_test.go (1)
go/node/audit/v1/errors.go (3)
ErrProviderNotFound(16-16)ErrAttributeNotFound(22-22)ErrInvalidAddress(19-19)
go/node/take/v1/errors_test.go (4)
go/node/market/v1/errors_test.go (1)
TestErrorGRPCStatusCodes(13-198)go/node/types/attributes/v1/errors_test.go (1)
TestErrorGRPCStatusCodes(13-38)go/node/types/deposit/v1/errors_test.go (1)
TestErrorGRPCStatusCodes(13-38)go/node/take/v1/errors.go (1)
ErrInvalidParam(14-14)
go/node/escrow/module/error_test.go (1)
go/node/escrow/module/error.go (15)
ErrAccountExists(29-29)ErrPaymentExists(34-34)ErrInvalidDenomination(33-33)ErrPaymentRateZero(37-37)ErrInvalidPayment(38-38)ErrInvalidSettlement(39-39)ErrInvalidID(40-40)ErrInvalidAccount(41-41)ErrInvalidAccountDepositor(42-42)ErrInvalidDeposit(44-44)ErrInvalidAuthzScope(45-45)ErrAccountClosed(30-30)ErrAccountOverdrawn(32-32)ErrPaymentClosed(35-35)ErrUnauthorizedDepositScope(43-43)
go/node/cert/v1/errors_test.go (3)
go/node/market/v1/errors_test.go (1)
TestErrorGRPCStatusCodes(13-198)go/node/take/v1/errors_test.go (1)
TestErrorGRPCStatusCodes(13-33)go/node/cert/v1/errors.go (9)
ErrCertificateNotFound(28-28)ErrCertificateExists(34-34)ErrInvalidAddress(31-31)ErrInvalidSerialNumber(40-40)ErrInvalidCertificateValue(43-43)ErrInvalidPubkeyValue(46-46)ErrInvalidState(49-49)ErrInvalidKeySize(52-52)ErrCertificateAlreadyRevoked(37-37)
go/node/audit/v1/errors.go (1)
go/node/types/attributes/v1/attribute.go (1)
ModuleName(18-18)
go/node/market/v1/errors_test.go (1)
go/node/market/v1/errors.go (34)
ErrUnknownBid(59-59)ErrUnknownLease(61-61)ErrUnknownLeaseForBid(63-63)ErrUnknownOrderForBid(65-65)ErrNoLeaseForOrder(73-73)ErrOrderNotFound(75-75)ErrGroupNotFound(77-77)ErrBidNotFound(83-83)ErrLeaseNotFound(87-87)ErrUnknownProvider(107-107)ErrBidExists(89-89)ErrOrderExists(97-97)ErrEmptyProvider(47-47)ErrSameAccount(49-49)ErrBidOverOrder(53-53)ErrAttributeMismatch(55-55)ErrCapabilitiesMismatch(57-57)ErrBidZeroPrice(85-85)ErrBidInvalidPrice(91-91)ErrInvalidDeposit(103-103)ErrInvalidParam(105-105)ErrInvalidBid(109-109)ErrInvalidLeaseClosedReason(111-111)ErrInvalidEscrowID(112-112)ErrLeaseNotActive(67-67)ErrBidNotActive(69-69)ErrBidNotOpen(71-71)ErrGroupNotOpen(79-79)ErrOrderNotOpen(81-81)ErrOrderActive(93-93)ErrOrderClosed(95-95)ErrOrderTooEarly(99-99)ErrOrderDurationExceeded(101-101)ErrInternal(51-51)
go/node/types/deposit/v1/errors.go (1)
go/node/types/attributes/v1/attribute.go (1)
ModuleName(18-18)
go/node/cert/v1/errors.go (1)
go/node/types/attributes/v1/attribute.go (1)
ModuleName(18-18)
go/node/types/deposit/v1/errors_test.go (3)
go/node/take/v1/errors_test.go (1)
TestErrorGRPCStatusCodes(13-33)go/node/types/attributes/v1/errors_test.go (1)
TestErrorGRPCStatusCodes(13-38)go/node/types/deposit/v1/errors.go (2)
ErrInvalidDepositor(17-17)ErrInvalidDepositSource(19-19)
go/node/types/attributes/v1/errors_test.go (1)
go/node/types/attributes/v1/attribute.go (2)
ErrAttributesDuplicateKeys(30-30)ErrInvalidAttributeKey(31-31)
go/node/provider/v1beta4/errors.go (1)
go/node/types/attributes/v1/attribute.go (1)
ModuleName(18-18)
go/node/escrow/module/error.go (1)
go/node/types/attributes/v1/attribute.go (1)
ModuleName(18-18)
go/node/market/v1/errors.go (1)
go/node/types/attributes/v1/attribute.go (1)
ModuleName(18-18)
go/node/deployment/v1/errors.go (1)
go/node/types/attributes/v1/attribute.go (1)
ModuleName(18-18)
go/node/provider/v1beta4/errors_test.go (1)
go/node/provider/v1beta4/errors.go (8)
ErrProviderNotFound(27-27)ErrProviderExists(30-30)ErrInvalidProviderURI(21-21)ErrNotAbsProviderURI(24-24)ErrInvalidAddress(33-33)ErrAttributes(36-36)ErrInvalidInfoWebsite(42-42)ErrIncompatibleAttributes(39-39)
⏰ 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). (4)
- GitHub Check: test
- GitHub Check: coverage
- GitHub Check: go
- GitHub Check: proto
🔇 Additional comments (24)
go/node/audit/v1/errors.go (1)
14-22: LGTM! gRPC code mappings are semantically correct.The error-to-gRPC-code assignments align well with standard conventions:
ErrProviderNotFoundandErrAttributeNotFound→codes.NotFoundErrInvalidAddress→codes.InvalidArgumentThis correctly addresses the HTTP 500 → 404 issue for "not found" semantics.
go/node/provider/v1beta4/errors.go (1)
19-43: LGTM! Comprehensive and semantically correct gRPC code mappings.The error registrations follow gRPC conventions properly:
- Validation errors (
ErrInvalidProviderURI,ErrNotAbsProviderURI,ErrInvalidAddress, etc.) →codes.InvalidArgument- Resource not found (
ErrProviderNotFound) →codes.NotFound- Duplicate resource (
ErrProviderExists) →codes.AlreadyExists- State violation (
ErrIncompatibleAttributes) →codes.FailedPreconditionThis ensures proper HTTP status code mapping via grpc-gateway.
go/node/deployment/v1/errors.go (2)
41-42: Key fix for issue #404: ErrDeploymentNotFound now maps to HTTP 404.This is the core change that resolves the bug. With
codes.NotFound, the grpc-gateway will correctly translate this to HTTP 404 instead of the previous HTTP 500.
34-81: Comprehensive gRPC code mappings look correct.All error-to-code mappings follow gRPC semantic conventions:
NotFoundfor missing resources (deployment, group, name)AlreadyExistsfor duplicate resourcesFailedPreconditionfor state violations (closed, paused, open states)InvalidArgumentfor validation errorsInternalfor internal errorsThe previous concern about
ErrInvalidGroupID(lines 59-60) has been addressed with the corrected comment and message.go/node/take/v1/errors.go (1)
12-15: LGTM!The
ErrInvalidParamcorrectly maps tocodes.InvalidArgument, which aligns with the semantic meaning of an invalid parameter.go/node/types/attributes/v1/errors_test.go (1)
13-38: LGTM! Well-structured test for gRPC status code mappings.The table-driven test pattern with subtests is clean and maintainable. The test correctly validates that:
- Errors are convertible to gRPC status via
status.FromError- The resulting status codes match expectations
This provides good regression protection for the error-to-HTTP-status mapping behavior.
go/node/take/v1/errors_test.go (1)
13-33: LGTM!Consistent test structure with other error test files. Validates the
ErrInvalidParam→codes.InvalidArgumentmapping.go/node/audit/v1/errors_test.go (1)
13-43: LGTM! Complete test coverage for audit/v1 errors.All three errors defined in
errors.goare covered with correct expected codes. The test structure is consistent with other modules in this PR.go/node/escrow/module/error_test.go (1)
13-113: LGTM! Comprehensive test coverage for escrow module errors.Excellent coverage of 16 error types across 5 different gRPC status codes (
NotFound,AlreadyExists,InvalidArgument,FailedPrecondition,PermissionDenied). The test cases align with the production error definitions inerror.go.go/node/market/v1/errors_test.go (1)
1-198: Comprehensive test coverage for market error mappings.The test correctly validates that all market module errors map to their intended gRPC status codes. The table-driven approach is clear and maintains consistency with error definitions in
errors.go.go/node/types/deposit/v1/errors_test.go (1)
1-38: Test coverage validated for deposit errors.The test correctly verifies that deposit-related errors map to
codes.InvalidArgument, aligning with the updated error registrations.go/node/cert/v1/errors.go (2)
7-7: Import added for gRPC codes.The addition of
google.golang.org/grpc/codesis necessary for the RegisterWithGRPCCode calls.
28-52: gRPC status codes correctly assigned to certificate errors.The error-to-code mappings are semantically appropriate:
NotFoundfor missing certificatesInvalidArgumentfor validation failuresAlreadyExistsfor duplicatesFailedPreconditionfor revoked certificatesThese changes will ensure certificate errors map to correct HTTP status codes (e.g., 404, 400, 409, 412) via gRPC-gateway.
go/node/provider/v1beta4/errors_test.go (1)
1-68: Provider error mappings validated.The test ensures provider module errors correctly convert to gRPC status codes, with appropriate mappings for not found, already exists, invalid input, and precondition failures.
go/node/cert/v1/errors_test.go (1)
1-73: Certificate error test coverage is complete.All certificate module errors are tested with correct gRPC code expectations, ensuring the changes in
errors.goare properly validated.go/node/types/deposit/v1/errors.go (2)
4-5: Imports updated for gRPC-aware error registration.The addition of
sdkerrorsalias andgrpc/codesenables explicit gRPC status code assignment.
17-19: Deposit errors correctly mapped to InvalidArgument.Both
ErrInvalidDepositorandErrInvalidDepositSourceappropriately map tocodes.InvalidArgument, which will translate to HTTP 400 for invalid request parameters.go/node/types/attributes/v1/attribute.go (2)
13-14: Imports added for gRPC code support.The sdkerrors alias and grpc/codes import enable explicit gRPC status code registration.
30-31: Attribute validation errors correctly mapped.Both attribute errors map to
codes.InvalidArgument, which is appropriate for validation failures (duplicate keys, invalid key format). These will correctly translate to HTTP 400.go/node/escrow/module/error.go (2)
4-5: Imports updated for gRPC-aware error handling.The switch to
sdkerrorsand addition ofgrpc/codesenables explicit status code mapping for escrow module errors.
29-45: Escrow error mappings are semantically correct.All 16 error registrations use appropriate gRPC codes:
NotFoundfor missing accounts/payments (→ HTTP 404)AlreadyExistsfor duplicates (→ HTTP 409)InvalidArgumentfor validation failures (→ HTTP 400)FailedPreconditionfor state violations (→ HTTP 412)PermissionDeniedfor authorization failures (→ HTTP 403)The use of
PermissionDeniedforErrUnauthorizedDepositScopeis particularly appropriate, as it distinguishes authorization failures from general validation errors.go/node/market/v1/errors.go (3)
3-6: LGTM!The import of
google.golang.org/grpc/codesis correctly added to support the gRPC code mappings.
41-43: LGTM!New error codes are correctly appended at the end of the iota sequence, preserving existing error code values for backward compatibility.
45-107: gRPC code mappings are semantically correct.The mappings appropriately use:
codes.NotFoundfor missing resources (bids, leases, orders, providers)codes.InvalidArgumentfor validation errorscodes.AlreadyExistsfor duplicate entriescodes.FailedPreconditionfor state-related errorscodes.Internalfor internal errorsThis correctly addresses the PR objective of returning HTTP 404 for not-found scenarios instead of HTTP 500.
vertex451
changed the title
vertex451
changed the title
|
this is consensus breaking change, will hold on merging for now |
…fix/500-instead-of-404
There was a problem hiding this comment.
Actionable comments posted: 2
Fix all issues with AI Agents 🤖
In @go/node/market/v1/errors.go:
- Line 77: The ErrGroupNotFound constant incorrectly registers the message
"order not found"; update the registration call for ErrGroupNotFound (using
sdkerrors.RegisterWithGRPCCode, errCodeGroupNotFound, and codes.NotFound) to use
the correct message "group not found" and, if present, update any surrounding
comment to match the "group" semantics so the name, message, and comment are
consistent.
- Line 63: ErrUnknownLeaseForBid is registered with errCodeUnknownLeaseForOrder
which mismatches the variable name and message; either add a new error code
constant named errCodeUnknownLeaseForBid and use it in
sdkerrors.RegisterWithGRPCCode for ErrUnknownLeaseForBid, or if the intent was
"order" rename ErrUnknownLeaseForBid to ErrUnknownLeaseForOrder (and update any
references) so the constant errCodeUnknownLeaseForOrder matches the error
variable and message.
🧹 Nitpick comments (1)
go/node/market/v1/errors.go (1)
112-112: Add comment for consistency with other error declarations.All other error variables have a preceding comment line, but
ErrInvalidEscrowIDis declared inline. For consistency and better documentation, add a comment on a separate line.🔎 Suggested fix
// ErrInvalidLeaseClosedReason indicates reason for lease close does not match context ErrInvalidLeaseClosedReason = sdkerrors.RegisterWithGRPCCode(ModuleName, errInvalidLeaseClosedReason, codes.InvalidArgument, "invalid lease closed reason") - ErrInvalidEscrowID = sdkerrors.RegisterWithGRPCCode(ModuleName, errInvalidEscrowID, codes.InvalidArgument, "invalid escrow id") + // ErrInvalidEscrowID indicates an invalid escrow identifier + ErrInvalidEscrowID = sdkerrors.RegisterWithGRPCCode(ModuleName, errInvalidEscrowID, codes.InvalidArgument, "invalid escrow id")
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
go/node/market/v1/errors.go
⏰ 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). (3)
- GitHub Check: coverage
- GitHub Check: test
- GitHub Check: go
🔇 Additional comments (2)
go/node/market/v1/errors.go (2)
5-5: LGTM! Import necessary for gRPC code constants.The addition of
google.golang.org/grpc/codesis required for theRegisterWithGRPCCodecalls throughout the file.
47-112: LGTM! gRPC code mappings are semantically correct.All error registrations use appropriate gRPC codes that align with gRPC best practices:
codes.InvalidArgumentfor validation and parameter errorscodes.NotFoundfor missing resourcescodes.FailedPreconditionfor state precondition failurescodes.AlreadyExistsfor duplicate resourcescodes.Internalfor internal errorsThis ensures proper HTTP status code mapping via grpc-gateway (e.g., NotFound → HTTP 404 instead of HTTP 500).
vertex451
changed the title
vertex451
changed the title
vertex451
force-pushed
the
artem/fix/500-instead-of-404
branch
from
6f31ee3 to
dac34e0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @go/node/market/v1/errors.go:
- Around line 78-79: The comment and error message for ErrGroupNotOpen are
inconsistent and reference "order" instead of "group": update the code where
ErrGroupNotOpen is registered (the sdkerrors.RegisterWithGRPCCode call that
defines ErrGroupNotOpen) so the inline comment reads something like
"ErrGroupNotOpen group not open" (or "group not open") and change the error
message string argument from "order not open" to "group not open" to match the
error name; ensure only the comment and the message string are changed, leaving
the registration call and GRPC code (codes.FailedPrecondition) intact.
🧹 Nitpick comments (2)
go/node/market/v1/errors.go (2)
80-81: Inconsistent message prefix inErrOrderNotOpen.The error message includes a "bid: " prefix ("bid: order not open") which is inconsistent with other error messages in the file. Most errors don't include such contextual prefixes in their base messages.
🔎 Suggested fix for consistency
- // ErrOrderNotOpen order not found - ErrOrderNotOpen = sdkerrors.RegisterWithGRPCCode(ModuleName, errCodeOrderNotOpen, codes.FailedPrecondition, "bid: order not open") + // ErrOrderNotOpen order not open + ErrOrderNotOpen = sdkerrors.RegisterWithGRPCCode(ModuleName, errCodeOrderNotOpen, codes.FailedPrecondition, "order not open")
103-103: Inconsistent capitalization in error message.The message "Deposit invalid" has a capital "D" which is inconsistent with other error messages in the file that use lowercase (e.g., "bid price is invalid", "parameter invalid").
🔎 Suggested fix for consistency
- ErrInvalidDeposit = sdkerrors.RegisterWithGRPCCode(ModuleName, errInvalidDeposit, codes.InvalidArgument, "Deposit invalid") + ErrInvalidDeposit = sdkerrors.RegisterWithGRPCCode(ModuleName, errInvalidDeposit, codes.InvalidArgument, "deposit invalid")
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
go/node/market/v1/errors.go
🧰 Additional context used
🧬 Code graph analysis (1)
go/node/market/v1/errors.go (1)
go/node/types/attributes/v1/attribute.go (1)
ModuleName(18-18)
⏰ 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). (4)
- GitHub Check: test
- GitHub Check: coverage
- GitHub Check: go
- GitHub Check: proto
🔇 Additional comments (4)
go/node/market/v1/errors.go (4)
5-5: LGTM: gRPC codes import.The addition of the
google.golang.org/grpc/codesimport is necessary and correct for the migration toRegisterWithGRPCCode.
16-16: Correctly adds missing constant.The addition of
errCodeUnknownLeaseForBidresolves the previous constant name mismatch whereErrUnknownLeaseForBidwas incorrectly usingerrCodeUnknownLeaseForOrder.
47-77: LGTM: Error registrations with appropriate gRPC codes.The error registrations correctly use
RegisterWithGRPCCodewith semantically appropriate gRPC codes:
codes.InvalidArgumentfor validation errorscodes.NotFoundfor missing entitiescodes.Internalfor internal errorscodes.FailedPreconditionfor state-based errorsThe previously flagged issues with
ErrInvalidBid,ErrUnknownLeaseForBid, andErrGroupNotFoundhave been successfully addressed.
83-112: LGTM: Remaining error registrations.The remaining error registrations correctly use
RegisterWithGRPCCodewith appropriate gRPC codes:
codes.NotFoundfor missing entitiescodes.InvalidArgumentfor validation errorscodes.FailedPreconditionfor state-based errorscodes.AlreadyExistsfor duplicate entities
vertex451
force-pushed
the
artem/fix/500-instead-of-404
branch
from
dac34e0 to
e57a675
Compare
… for the proper error propagation. (#173)
3 participants
📝 Description
Register function that was used in our error registering under the hood uses grpccodes.Unknown code, which is mapped to http.InternalError status code(500). link
So, each error ended up with that code at this line:
My fix replaces Register with RegisterWithGRPCCode that propagates the original error to the http status code.
Testing
Tested at local network with akash binary build using chain-sdk with this branch:
In browser you can see code 404, which mean that the bug is fixed.
Question to resolve
🔧 Purpose of the Change
📌 Related Issues
✅ Checklist