Remove Redundant Staking Errors #9231

jeebster · 2021-04-29T18:11:44Z

Description

closes: #5450

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer
Review Codecov Report in the comment section below once CI passes

codecov · 2021-05-01T02:22:38Z

Codecov Report

Merging #9231 (d5d99fe) into master (9d05048) will decrease coverage by 0.07%.
The diff coverage is 51.02%.

❗ Current head d5d99fe differs from pull request most recent head 986fa22. Consider uploading reports for the commit 986fa22 to get more accurate results

@@            Coverage Diff             @@
##           master    #9231      +/-   ##
==========================================
- Coverage   60.38%   60.30%   -0.08%     
==========================================
  Files         587      588       +1     
  Lines       36920    36999      +79     
==========================================
+ Hits        22294    22313      +19     
- Misses      12651    12710      +59     
- Partials     1975     1976       +1

Impacted Files	Coverage Δ
x/staking/client/cli/tx.go	`17.98% <0.00%> (ø)`
x/staking/keeper/delegation.go	`70.73% <0.00%> (ø)`
x/staking/keeper/msg_server.go	`0.43% <0.00%> (-0.02%)`	⬇️
store/rootmulti/store.go	`68.23% <50.00%> (-0.24%)`	⬇️
x/staking/types/msg.go	`57.32% <77.27%> (+2.39%)`	⬆️
store/cachemulti/store.go	`11.32% <80.00%> (ø)`
x/staking/client/testutil/suite.go	`99.49% <100.00%> (ø)`

x/staking/client/cli/tx.go

alexanderbez · 2021-05-10T12:39:53Z

x/staking/client/cli/tx.go

-					return types.ErrMinSelfDelegationInvalid
+					return sdkerrors.Wrap(
+						sdkerrors.ErrInvalidRequest,
+						"minimum self delegation must be a positive integer",


s/positive/valid

Sorry, not sure what you mean by this...would you like the message argument rephrased?

alexanderbez · 2021-05-10T12:40:02Z

x/staking/client/cli/tx.go

-		return txBldr, nil, types.ErrMinSelfDelegationInvalid
+		return txBldr, nil, sdkerrors.Wrap(
+			sdkerrors.ErrInvalidRequest,
+			"minimum self delegation must be a positive integer",


s/positive/valid

See aforementioned

alexanderbez · 2021-05-10T12:40:21Z

x/staking/keeper/delegation.go

+		return shares, sdkerrors.Wrap(
+			sdkerrors.ErrInvalidRequest,
+			"invalid shares amount",
+		)


There is no need for all the wrapping. Can we have a single line?

Sure thing. Is there support for such formatting via the package linters/formatters so this can be automated for all contributors?

Addressed in 8c69a6f3c

alexanderbez · 2021-05-10T12:40:50Z

x/staking/types/msg.go

+		return sdkerrors.Wrap(
+			sdkerrors.ErrInvalidRequest,
+			"validator address is invalid",
+		)


nit: single line :)

See aforementioned

Addressed in 8c69a6f3c

alexanderbez · 2021-05-10T12:40:58Z

x/staking/types/msg.go

+		return sdkerrors.Wrap(
+			sdkerrors.ErrInvalidRequest,
+			"invalid delegation amount",
+		)


single line :)

See aforementioned

Addressed in 8c69a6f3c

robert-zaremba · 2021-05-16T18:20:11Z

x/staking/client/cli/tx.go

@@ -122,7 +123,7 @@ func NewEditValidatorCmd() *cobra.Command {
 			if minSelfDelegationString != "" {
 				msb, ok := sdk.NewIntFromString(minSelfDelegationString)
 				if !ok {
-					return types.ErrMinSelfDelegationInvalid
+					return sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "minimum self delegation must be a positive integer")


you can use the wrap method of the error, it makes the code a bit shorter

sdkerrors.ErrInvalidRequest.Wrap("minimum self delegation must be a positive integer"

Same in other places.

@robert-zaremba Thanks for bringing this to my attention, I'll take a look at the Wrap() implementation. I see both implementation strategies in the repository, please let me know which is preferred and whether refactoring should be executed accordingly

I think this is more of a nit. We use the design currently coded throughout the repo and the one robert mentioned in a few places.

@robert-zaremba want to approve this?

I'm not blocking it - I only made a suggestion. The PR already got enough approvals.

BTW - we implemented the Wrap method for the error types 2-3 months ago, that why it's not use that much.

…move unused error types

…ctor

* refactor(staking errors): 'invalid' errors: use ErrInvalidRequest, remove unused error types * refactor(staking errors): fix error registration codes * support(staking errors): add changelog entry * fix(staking test suite): update expected error codes relative to refactor * chore(staking errors): code formatting Co-authored-by: Marko <marbar3778@yahoo.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

github-actions bot added C:CLI C:x/staking labels Apr 29, 2021

jeebster mentioned this pull request Apr 29, 2021

Remove Redundant Staking Errors #5450

Closed

4 tasks

jeebster force-pushed the jeebster/5450-staking-errors-refactor branch from 30d5bfb to f76023b Compare May 1, 2021 00:43

jeebster force-pushed the jeebster/5450-staking-errors-refactor branch 2 times, most recently from 3d237c2 to f0017fa Compare May 7, 2021 15:02

jeebster marked this pull request as ready for review May 7, 2021 21:22

jeebster requested review from aaronc and alexanderbez as code owners May 7, 2021 21:22

alexanderbez reviewed May 10, 2021

View reviewed changes

cyberbono3 assigned amaury1093 May 10, 2021

jeebster force-pushed the jeebster/5450-staking-errors-refactor branch from f0017fa to 8c69a6f Compare May 13, 2021 00:38

alexanderbez approved these changes May 13, 2021

View reviewed changes

tac0turtle requested review from amaury1093, blushi and robert-zaremba May 14, 2021 06:36

tac0turtle approved these changes May 14, 2021

View reviewed changes

robert-zaremba reviewed May 16, 2021

View reviewed changes

jeebster added 5 commits May 18, 2021 18:31

refactor(staking errors): 'invalid' errors: use ErrInvalidRequest, re…

bc52259

…move unused error types

refactor(staking errors): fix error registration codes

6cfd058

support(staking errors): add changelog entry

c3b4b62

fix(staking test suite): update expected error codes relative to refa…

b721616

…ctor

chore(staking errors): code formatting

4914118

jeebster force-pushed the jeebster/5450-staking-errors-refactor branch from 8c69a6f to 4914118 Compare May 19, 2021 00:32

Merge branch 'master' into jeebster/5450-staking-errors-refactor

d5d99fe

tac0turtle added the A:automerge Automatically merge PR once all prerequisites pass. label May 19, 2021

mergify bot added 2 commits May 19, 2021 10:22

Merge branch 'master' into jeebster/5450-staking-errors-refactor

80c36e5

Merge branch 'master' into jeebster/5450-staking-errors-refactor

986fa22

mergify bot merged commit ad49ec1 into cosmos:master May 19, 2021

jeebster deleted the jeebster/5450-staking-errors-refactor branch May 25, 2021 00:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove Redundant Staking Errors #9231

Remove Redundant Staking Errors #9231

jeebster commented Apr 29, 2021 •

edited by tac0turtle

Loading

codecov bot commented May 1, 2021 •

edited

Loading

alexanderbez May 10, 2021

jeebster May 10, 2021

alexanderbez May 10, 2021

jeebster May 10, 2021

alexanderbez May 10, 2021

jeebster May 10, 2021

jeebster May 13, 2021

alexanderbez May 10, 2021

jeebster May 10, 2021

jeebster May 13, 2021

alexanderbez May 10, 2021

jeebster May 10, 2021

jeebster May 13, 2021

robert-zaremba May 16, 2021

jeebster May 17, 2021 •

edited

Loading

tac0turtle May 18, 2021

robert-zaremba May 18, 2021

robert-zaremba May 18, 2021

Remove Redundant Staking Errors #9231

Remove Redundant Staking Errors #9231

Conversation

jeebster commented Apr 29, 2021 • edited by tac0turtle Loading

Description

codecov bot commented May 1, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeebster May 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeebster commented Apr 29, 2021 •

edited by tac0turtle

Loading

codecov bot commented May 1, 2021 •

edited

Loading

jeebster May 17, 2021 •

edited

Loading