-
Notifications
You must be signed in to change notification settings - Fork 198
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Standardize the error codes from API endpoints #317
Changes from 7 commits
6d90d77
eef17ae
cc56061
1e26e44
5cda3f3
03a4fbe
52d8bd7
5452188
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
package common | ||
|
||
import ( | ||
"google.golang.org/grpc/codes" | ||
"google.golang.org/grpc/status" | ||
) | ||
|
||
// The canonical errors from the EigenDA gRPC API endpoints. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need one for timeout? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We may add if it's becoming a more clear case to cover |
||
// | ||
// Notes: | ||
// - Start the error space small (but sufficient), and expand when there is an important | ||
// failure case to separate out. | ||
// - Avoid simply wrapping system-internal errors without checking if they are appropriate | ||
// in user-facing errors defined here. Consider map and convert system-internal errors | ||
// before return to users from APIs. | ||
|
||
func NewGRPCError(code codes.Code, msg string) error { | ||
return status.Errorf(code, msg) | ||
} | ||
|
||
// HTTP Mapping: 400 Bad Request | ||
func NewInvalidArgError(msg string) error { | ||
return NewGRPCError(codes.InvalidArgument, msg) | ||
} | ||
|
||
// HTTP Mapping: 404 Not Found | ||
func NewNotFoundError(msg string) error { | ||
return NewGRPCError(codes.NotFound, msg) | ||
} | ||
|
||
// HTTP Mapping: 429 Too Many Requests | ||
func NewResourceExhaustedError(msg string) error { | ||
return NewGRPCError(codes.ResourceExhausted, msg) | ||
} | ||
|
||
// HTTP Mapping: 500 Internal Server Error | ||
func NewInternalError(msg string) error { | ||
return NewGRPCError(codes.Internal, msg) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -93,12 +93,12 @@ func (s *DispersalServer) DisperseBlobAuthenticated(stream pb.Disperser_Disperse | |
// Process disperse_request | ||
in, err := stream.Recv() | ||
if err != nil { | ||
return fmt.Errorf("error receiving next message: %v", err) | ||
return common.NewInvalidArgError(fmt.Sprintf("error receiving next message: %v", err)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Is this really due to invalid argument? Feel like stream.Revc() breaking could be due to pod dying in the middle or something like that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good point, this may be not a clear cut whether it's client side or server side issue, as it may be caused by various reasons, like network connection, malformed message as well as server side resource issue etc. |
||
} | ||
|
||
request, ok := in.Payload.(*pb.AuthenticatedRequest_DisperseRequest) | ||
if !ok { | ||
return errors.New("expected DisperseBlobRequest") | ||
return common.NewInvalidArgError("missing DisperseBlobRequest") | ||
} | ||
|
||
blob := getBlobFromRequest(request.DisperseRequest) | ||
|
@@ -107,12 +107,12 @@ func (s *DispersalServer) DisperseBlobAuthenticated(stream pb.Disperser_Disperse | |
// Decode public key | ||
publicKeyBytes, err := hexutil.Decode(blob.RequestHeader.AccountID) | ||
if err != nil { | ||
return fmt.Errorf("failed to decode public key (%v): %v", blob.RequestHeader.AccountID, err) | ||
return common.NewInvalidArgError(fmt.Sprintf("failed to decode public key (%v): %v", blob.RequestHeader.AccountID, err)) | ||
} | ||
|
||
pubKey, err := crypto.UnmarshalPubkey(publicKeyBytes) | ||
if err != nil { | ||
return fmt.Errorf("failed to decode public key (%v): %v", blob.RequestHeader.AccountID, err) | ||
return common.NewInvalidArgError(fmt.Sprintf("failed to decode public key (%v): %v", blob.RequestHeader.AccountID, err)) | ||
} | ||
|
||
authenticatedAddress := crypto.PubkeyToAddress(*pubKey).String() | ||
|
@@ -131,20 +131,20 @@ func (s *DispersalServer) DisperseBlobAuthenticated(stream pb.Disperser_Disperse | |
// Recieve challenge_reply | ||
in, err = stream.Recv() | ||
if err != nil { | ||
return fmt.Errorf("error receiving next message: %v", err) | ||
return common.NewInvalidArgError(fmt.Sprintf("error receiving next message: %v", err)) | ||
} | ||
|
||
challengeReply, ok := in.Payload.(*pb.AuthenticatedRequest_AuthenticationData) | ||
if !ok { | ||
return errors.New("expected AuthenticationData") | ||
return common.NewInvalidArgError("expected AuthenticationData") | ||
} | ||
|
||
blob.RequestHeader.Nonce = challenge | ||
blob.RequestHeader.AuthenticationData = challengeReply.AuthenticationData.AuthenticationData | ||
|
||
err = s.authenticator.AuthenticateBlobRequest(blob.RequestHeader.BlobAuthHeader) | ||
if err != nil { | ||
return fmt.Errorf("failed to authenticate blob request: %v", err) | ||
return common.NewInvalidArgError(fmt.Sprintf("failed to authenticate blob request: %v", err)) | ||
} | ||
|
||
// Disperse the blob | ||
|
@@ -247,7 +247,7 @@ func (s *DispersalServer) disperseBlob(ctx context.Context, blob *core.Blob, aut | |
quorumId := string(param.QuorumID) | ||
s.metrics.HandleFailedRequest(quorumId, blobSize, "DisperseBlob") | ||
} | ||
return nil, err | ||
return nil, common.NewInvalidArgError(err.Error()) | ||
} | ||
|
||
s.logger.Debug("received a new blob request", "origin", origin, "securityParams", strings.Join(securityParamsStrings, ", ")) | ||
|
@@ -258,7 +258,7 @@ func (s *DispersalServer) disperseBlob(ctx context.Context, blob *core.Blob, aut | |
quorumId := string(param.QuorumID) | ||
s.metrics.HandleFailedRequest(quorumId, blobSize, "DisperseBlob") | ||
} | ||
return nil, err | ||
return nil, common.NewInvalidArgError(err.Error()) | ||
} | ||
|
||
if s.ratelimiter != nil { | ||
|
@@ -274,7 +274,7 @@ func (s *DispersalServer) disperseBlob(ctx context.Context, blob *core.Blob, aut | |
s.metrics.HandleFailedRequest(quorumId, blobSize, "DisperseBlob") | ||
} | ||
} | ||
return nil, err | ||
return nil, common.NewResourceExhaustedError(err.Error()) | ||
} | ||
} | ||
|
||
|
@@ -286,7 +286,7 @@ func (s *DispersalServer) disperseBlob(ctx context.Context, blob *core.Blob, aut | |
s.metrics.HandleBlobStoreFailedRequest(quorumId, blobSize, "DisperseBlob") | ||
} | ||
s.logger.Error("failed to store blob", "err", err) | ||
return nil, errors.New("failed to store blob, please try again later") | ||
return nil, common.NewInternalError("failed to store blob, please try again later") | ||
} | ||
|
||
for _, param := range securityParams { | ||
|
@@ -443,24 +443,25 @@ func (s *DispersalServer) GetBlobStatus(ctx context.Context, req *pb.BlobStatusR | |
|
||
requestID := req.GetRequestId() | ||
if len(requestID) == 0 { | ||
return nil, errors.New("invalid request: request_id must not be empty") | ||
return nil, common.NewInvalidArgError("request_id must not be empty") | ||
} | ||
|
||
s.logger.Info("received a new blob status request", "requestID", string(requestID)) | ||
metadataKey, err := disperser.ParseBlobKey(string(requestID)) | ||
if err != nil { | ||
return nil, err | ||
return nil, common.NewInvalidArgError(fmt.Sprintf("failed to parse the requestID: %s", err.Error())) | ||
} | ||
|
||
s.logger.Debug("metadataKey", "metadataKey", metadataKey.String()) | ||
metadata, err := s.blobStore.GetBlobMetadata(ctx, metadataKey) | ||
if err != nil { | ||
return nil, err | ||
// TODO: we need to distinguish NOT_FOUND from actual internal error. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would probably need to standarize the error message and then check that here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, we could check the message, or surface an error with |
||
return nil, common.NewInternalError(fmt.Sprintf("failed to get blob metadata, blobkey: %s", metadataKey.String())) | ||
} | ||
|
||
isConfirmed, err := metadata.IsConfirmed() | ||
if err != nil { | ||
return nil, err | ||
return nil, common.NewInternalError(fmt.Sprintf("missing confirmation information: %s", err.Error())) | ||
} | ||
|
||
s.logger.Debug("isConfirmed", "metadata", metadata, "isConfirmed", isConfirmed) | ||
|
@@ -556,15 +557,16 @@ func (s *DispersalServer) RetrieveBlob(ctx context.Context, req *pb.RetrieveBlob | |
s.logger.Error("Failed to retrieve blob metadata", "err", err) | ||
s.metrics.IncrementFailedBlobRequestNum("", "RetrieveBlob") | ||
|
||
return nil, err | ||
// TODO: we need to distinguish NOT_FOUND from actual internal error. | ||
return nil, common.NewInternalError("failed to get blob metadata, please retry") | ||
} | ||
|
||
data, err := s.blobStore.GetBlobContent(ctx, blobMetadata.BlobHash) | ||
if err != nil { | ||
s.logger.Error("Failed to retrieve blob", "err", err) | ||
s.metrics.HandleFailedRequest("", len(data), "RetrieveBlob") | ||
|
||
return nil, err | ||
return nil, common.NewInternalError("failed to get blob data, please retry") | ||
} | ||
|
||
s.metrics.HandleSuccessfulRequest("", len(data), "RetrieveBlob") | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this just go in the
api
package?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point! The only question is
api
is currently all protobuf stuff, it otherwise fits better inapi
since these are all around APIs. I'll move it over unless other folks have different thought/suggestion.