Skip to content
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.

Sign up for GitHub

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

Minor Error Message Cleanup #3691

Merged
merged 5 commits into from
Feb 21, 2019
Merged

Minor Error Message Cleanup #3691

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d1a8f32
cleanup error messages
alexanderbez Feb 20, 2019
e4a7330
Add pending log entry
alexanderbez Feb 20, 2019
3c4a775
Rename Message to Error in ErrorResponse
alexanderbez Feb 20, 2019
76af272
Merge branch 'develop' into bez/err-message-cleanup
alexanderbez Feb 20, 2019
5cd1c1b
Merge branch 'develop' into bez/err-message-cleanup
rigelrozanski Feb 21, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion PENDING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,10 @@

### SDK

* [\#3665] Overhaul sdk.Uint type in preparation for Coins Int -> Uint migration.
* \#3691 Cleanup error messages
* \#3456 Integrate in the Int.ToDec() convenience function
* [\#3300] Update the spec-spec, spec file reorg, and TOC updates.
* [\#3665] Overhaul sdk.Uint type in preparation for Coins's Int -> Uint migration.

### Tendermint

Expand Down
8 changes: 4 additions & 4 deletions types/rest/rest.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,13 +111,13 @@ func ReadRESTReq(w http.ResponseWriter, r *http.Request, cdc *codec.Codec, req i

// ErrorResponse defines the attributes of a JSON error response.
type ErrorResponse struct {
Code int `json:"code,omitempty"`
Message string `json:"message"`
Code int `json:"code,omitempty"`
Error string `json:"error"`
Copy link
Contributor Author

@alexanderbez alexanderbez Feb 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ref: #3693 -- this should always be valid JSON @alessio. Thoughts?

/cc @faboweb

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes plz!

}

// NewErrorResponse creates a new ErrorResponse instance.
func NewErrorResponse(code int, msg string) ErrorResponse {
return ErrorResponse{Code: code, Message: msg}
func NewErrorResponse(code int, err string) ErrorResponse {
return ErrorResponse{Code: code, Error: err}
}

// WriteErrorResponse prepares and writes a HTTP error
Expand Down
2 changes: 1 addition & 1 deletion x/auth/ante.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ func GetSignerAcc(ctx sdk.Context, ak AccountKeeper, addr sdk.AccAddress) (Accou
if acc := ak.GetAccount(ctx, addr); acc != nil {
return acc, sdk.Result{}
}
return nil, sdk.ErrUnknownAddress(addr.String()).Result()
return nil, sdk.ErrUnknownAddress(fmt.Sprintf("account %s does not exist", addr)).Result()
}

// ValidateMemo validates the memo size.
Expand Down
8 changes: 5 additions & 3 deletions x/auth/keeper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package auth

import (
"fmt"

"github.com/tendermint/tendermint/crypto"

codec "github.com/cosmos/cosmos-sdk/codec"
Expand Down Expand Up @@ -143,7 +145,7 @@ func (ak AccountKeeper) IterateAccounts(ctx sdk.Context, process func(Account) (
func (ak AccountKeeper) GetPubKey(ctx sdk.Context, addr sdk.AccAddress) (crypto.PubKey, sdk.Error) {
acc := ak.GetAccount(ctx, addr)
if acc == nil {
return nil, sdk.ErrUnknownAddress(addr.String())
return nil, sdk.ErrUnknownAddress(fmt.Sprintf("account %s does not exist", addr))
}
return acc.GetPubKey(), nil
}
Expand All @@ -152,15 +154,15 @@ func (ak AccountKeeper) GetPubKey(ctx sdk.Context, addr sdk.AccAddress) (crypto.
func (ak AccountKeeper) GetSequence(ctx sdk.Context, addr sdk.AccAddress) (uint64, sdk.Error) {
acc := ak.GetAccount(ctx, addr)
if acc == nil {
return 0, sdk.ErrUnknownAddress(addr.String())
return 0, sdk.ErrUnknownAddress(fmt.Sprintf("account %s does not exist", addr))
}
return acc.GetSequence(), nil
}

func (ak AccountKeeper) setSequence(ctx sdk.Context, addr sdk.AccAddress, newSequence uint64) sdk.Error {
acc := ak.GetAccount(ctx, addr)
if acc == nil {
return sdk.ErrUnknownAddress(addr.String())
return sdk.ErrUnknownAddress(fmt.Sprintf("account %s does not exist", addr))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the ErrUnknownAddress take an addr and print out the error in the correct format?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, but that would be break the general convention of these error functions where they all just take a simple msg string param.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, not really crazy about that pattern. It seems to leave a lot of subtly different errors for the same issues. I think we should standardize these, but that may be out of the scope of the PR.

}

if err := acc.SetSequence(newSequence); err != nil {
Expand Down
20 changes: 11 additions & 9 deletions x/staking/querier/querier.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package querier

import (
"fmt"

abci "github.com/tendermint/tendermint/abci/types"

"github.com/cosmos/cosmos-sdk/codec"
Expand Down Expand Up @@ -142,7 +144,7 @@ func queryValidator(ctx sdk.Context, cdc *codec.Codec, req abci.RequestQuery, k

errRes := cdc.UnmarshalJSON(req.Data, &params)
if errRes != nil {
return []byte{}, sdk.ErrUnknownAddress("")
return []byte{}, sdk.ErrInternal(fmt.Sprintf("failed to parse params: %s", err))
}

validator, found := k.GetValidator(ctx, params.ValidatorAddr)
Expand All @@ -162,7 +164,7 @@ func queryValidatorDelegations(ctx sdk.Context, cdc *codec.Codec, req abci.Reque

errRes := cdc.UnmarshalJSON(req.Data, &params)
if errRes != nil {
return []byte{}, sdk.ErrUnknownAddress("")
return []byte{}, sdk.ErrInternal(fmt.Sprintf("failed to parse params: %s", err))
}

delegations := k.GetValidatorDelegations(ctx, params.ValidatorAddr)
Expand All @@ -179,7 +181,7 @@ func queryValidatorUnbondingDelegations(ctx sdk.Context, cdc *codec.Codec, req a

errRes := cdc.UnmarshalJSON(req.Data, &params)
if errRes != nil {
return []byte{}, sdk.ErrUnknownAddress("")
return []byte{}, sdk.ErrInternal(fmt.Sprintf("failed to parse params: %s", err))
}

unbonds := k.GetUnbondingDelegationsFromValidator(ctx, params.ValidatorAddr)
Expand All @@ -196,7 +198,7 @@ func queryDelegatorDelegations(ctx sdk.Context, cdc *codec.Codec, req abci.Reque

errRes := cdc.UnmarshalJSON(req.Data, &params)
if errRes != nil {
return []byte{}, sdk.ErrUnknownAddress("")
return []byte{}, sdk.ErrInternal(fmt.Sprintf("failed to parse params: %s", err))
}

delegations := k.GetAllDelegatorDelegations(ctx, params.DelegatorAddr)
Expand All @@ -213,7 +215,7 @@ func queryDelegatorUnbondingDelegations(ctx sdk.Context, cdc *codec.Codec, req a

errRes := cdc.UnmarshalJSON(req.Data, &params)
if errRes != nil {
return []byte{}, sdk.ErrUnknownAddress("")
return []byte{}, sdk.ErrInternal(fmt.Sprintf("failed to parse params: %s", err))
}

unbondingDelegations := k.GetAllUnbondingDelegations(ctx, params.DelegatorAddr)
Expand All @@ -232,7 +234,7 @@ func queryDelegatorValidators(ctx sdk.Context, cdc *codec.Codec, req abci.Reques

errRes := cdc.UnmarshalJSON(req.Data, &params)
if errRes != nil {
return []byte{}, sdk.ErrUnknownAddress("")
return []byte{}, sdk.ErrInternal(fmt.Sprintf("failed to parse params: %s", err))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe sdk.ErrFailedParams or something similar?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, I'm not sure that is totally a good idea as we could have a multitude of decoding failures and we don't want a unique failure type/function for reach. Maybe ErrFailedDecode but that can be for another PR.

}

validators := k.GetDelegatorValidators(ctx, params.DelegatorAddr, stakingParams.MaxValidators)
Expand All @@ -249,7 +251,7 @@ func queryDelegatorValidator(ctx sdk.Context, cdc *codec.Codec, req abci.Request

errRes := cdc.UnmarshalJSON(req.Data, &params)
if errRes != nil {
return []byte{}, sdk.ErrUnknownAddress("")
return []byte{}, sdk.ErrInternal(fmt.Sprintf("failed to parse params: %s", err))
}

validator, err := k.GetDelegatorValidator(ctx, params.DelegatorAddr, params.ValidatorAddr)
Expand All @@ -269,7 +271,7 @@ func queryDelegation(ctx sdk.Context, cdc *codec.Codec, req abci.RequestQuery, k

errRes := cdc.UnmarshalJSON(req.Data, &params)
if errRes != nil {
return []byte{}, sdk.ErrUnknownAddress("")
return []byte{}, sdk.ErrInternal(fmt.Sprintf("failed to parse params: %s", err))
}

delegation, found := k.GetDelegation(ctx, params.DelegatorAddr, params.ValidatorAddr)
Expand All @@ -289,7 +291,7 @@ func queryUnbondingDelegation(ctx sdk.Context, cdc *codec.Codec, req abci.Reques

errRes := cdc.UnmarshalJSON(req.Data, &params)
if errRes != nil {
return []byte{}, sdk.ErrUnknownAddress("")
return []byte{}, sdk.ErrInternal(fmt.Sprintf("failed to parse params: %s", err))
}

unbond, found := k.GetUnbondingDelegation(ctx, params.DelegatorAddr, params.ValidatorAddr)
Expand Down