Global error refactor and docs #339

ruseinov · 2019-02-20T18:08:25Z

fixes #309
fixes #333

codecov-io · 2019-02-20T18:17:22Z

Codecov Report

Merging #339 into master will decrease coverage by 1.2%.
The diff coverage is 53.8%.

@@           Coverage Diff            @@
##           master    #339     +/-   ##
========================================
- Coverage    70.4%   69.1%   -1.3%     
========================================
  Files         131     129      -2     
  Lines        6208    6209      +1     
========================================
- Hits         4374    4295     -79     
- Misses       1414    1474     +60     
- Partials      420     440     +20

Impacted Files	Coverage Δ
errors/errors.go	`62% <ø> (-5.2%)`	⬇️
x/sigs/decorator.go	`94.7% <ø> (ø)`	⬆️
errors/main.go	`0% <ø> (-96.7%)`	⬇️
x/currency/msg.go	`20% <0%> (ø)`	⬆️
x/currency/model.go	`76.9% <0%> (ø)`	⬆️
examples/mycoind/app/tx.go	`68.1% <0%> (ø)`	⬆️
x/paychan/msg.go	`37.7% <0%> (ø)`	⬆️
examples/tutorial/x/blog/models.go	`63.1% <0%> (ø)`	⬆️
x/escrow/model.go	`66.2% <0%> (ø)`	⬆️
app/commit.go	`68.4% <0%> (ø)`	⬆️
... and 49 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d848b60...151e6a1. Read the comment docs.

ruseinov · 2019-02-20T20:53:28Z

@ethanfrey this approach is a bit radical, I tried to reuse as many errors as possible from the common package and I think it looks ok, especially considering that blog is not something you necessarily need to do using weave. Let's see what you think

ethanfrey

Generally nice.

Wondering about need of ErrExists, rather than ErrDuplicate, which is why not approved.

Curious about benefits of not inlining the Printf strings... please just make a good argument why not (which can go in the best practices readme)

ethanfrey · 2019-02-21T10:06:16Z

errors/errors.go

+	// ErrInvalidInput stands for general input problems indication
+	ErrInvalidInput = Register(14, "invalid input")
+
+	// ErrExists already exists error


What is the distinction between ErrExists and ErrDuplicate?
I guess they could be merged

ethanfrey · 2019-02-21T10:07:50Z

examples/tutorial/x/blog/handlers.go

@@ -90,7 +90,7 @@ func (h CreateBlogMsgHandler) validate(ctx weave.Context, db weave.KVStore, tx w
 	// Retrieve tx main signer in this context
 	sender := x.MainSigner(ctx, h.auth)
 	if sender == nil {
-		return nil, ErrUnauthorisedBlogAuthor(nil)
+		return nil, errors.ErrUnauthorized.Newf(unauthorisedBlogAuthor, nil)


This is odd to me, to see a Printf type function, but not the string, so I don't even know if the next arguments are correct. Can you inline these definitions that use Newf.

For those using New, it is your call to inline or keep them in vars.

ethanfrey · 2019-02-21T10:08:02Z

examples/tutorial/x/blog/errors.go

-	errBlogNotFound      = fmt.Errorf("No blog found for post")
-	errBlogExist         = fmt.Errorf("Blog already exists")
-	errBlogOneAuthorLeft = fmt.Errorf("Unable to remove last blog author")
+	invalidTitle           = "title is too long or too short"


These can all be const, right?

ethanfrey · 2019-02-21T10:08:19Z

examples/tutorial/x/blog/handlers.go

@@ -198,7 +198,7 @@ func (h CreatePostMsgHandler) validate(ctx weave.Context, db weave.KVStore, tx w
 		return nil, nil, err
 	}
 	if obj == nil || (obj != nil && obj.Value() == nil) {
-		return nil, nil, ErrBlogNotFound()
+		return nil, nil, errors.ErrNotFound.New("blog")


👍 nice reuse

ethanfrey · 2019-02-21T10:08:43Z

examples/tutorial/x/blog/handlers.go

 	}

 	// Get the author index
 	authorIdx := findAuthor(blog.Authors, changeBlogAuthorsMsg.Author)
 	if changeBlogAuthorsMsg.Add {
 		// When removing an author we must ensure it does not exist already
 		if authorIdx >= 0 {
-			return nil, nil, ErrAuthorAlreadyExist(changeBlogAuthorsMsg.Author)
+			return nil, nil, errors.ErrExists.Newf("author: %X", changeBlogAuthorsMsg.Author)


This could be a duplicate error, right?

yes, let's use this. I was looking for exists error and duplicate didn't come up for some reason.

ethanfrey · 2019-02-21T10:09:26Z

examples/tutorial/x/blog/handlers.go

 	}

 	blog := obj.Value().(*Blog)
 	// Check main signer is one of the blog authors
 	if findAuthor(blog.Authors, sender.Address()) == -1 {
-		return nil, nil, ErrUnauthorisedBlogAuthor(sender.Address())
+		return nil, nil, errors.ErrUnauthorized.Newf(unauthorisedBlogAuthor, sender.Address())


Okay, I have now seen around 4 uses of unauthorizedBlogAuthor, and see why you extracted it.
Easier to change, but harder to read.

I dunna, ambivilent around it

I reuse format strings for a reason, mainly when there are too many occurrences.
What I will do is rename it to blahBlahFormat

ethanfrey

Great job. Made a few comments to polish it, then ready to merge.

Can you do one more pass then ping me so. I can approve?

ethanfrey · 2019-02-22T13:35:43Z

abci_test.go

-		{errors.New("nonce", errors.CodeUnauthorized), "nonce", errors.CodeUnauthorized},
-		{errors.WithCode(fmt.Errorf("no sender"), errors.CodeUnrecognizedAddress), "no sender", errors.CodeUnrecognizedAddress},
-		{errors.ErrDecoding(), errors.ErrDecoding().Error(), errors.CodeTxParseError},
+		{errors.NormalizePanic("stdlib"), "internal", errors.ErrInternal.ABCICode()},


Yes, smaller set of options to create them

ethanfrey · 2019-02-22T13:36:54Z

app/router.go

 }

 // Deliver always returns ErrNoSuchPath
 func (h noSuchPathHandler) Deliver(ctx weave.Context, store weave.KVStore,
 	tx weave.Tx) (weave.DeliverResult, error) {

-	return weave.DeliverResult{}, ErrNoSuchPath(h.path)
+	return weave.DeliverResult{}, errors.ErrNotFound.Newf("path: %s", h.path)


I like how these are rewritten. Standard error class, with some useful text.
👍

ethanfrey · 2019-02-22T13:39:24Z

app/store.go

@@ -111,7 +111,7 @@ func (s *StoreApp) parseAppState(data []byte, chainID string, init weave.Initial
 	var appState weave.Options
 	err := json.Unmarshal(data, &appState)
 	if err != nil {
-		return errors.WithCode(err, errors.CodeTxParseError)
+		return errors.ErrInternal.New(err.Error())


I would prefer

errors.Wrap(err, "json decoding")

It will get internal error code automatically, and seems simpler if we just want to return the raw error with ErrInternal

ethanfrey · 2019-02-22T13:41:14Z

cmd/bnsd/x/nft/username/handler.go

@@ -177,10 +177,10 @@ func (h RemoveChainAddressHandler) Deliver(ctx weave.Context, store weave.KVStor

 	actor := nft.FindActor(h.auth, ctx, t, nft.UpdateDetails)
 	if actor == nil {
-		return res, errors.ErrUnauthorizedLegacy()
+		return res, errors.ErrUnauthorized


I would like a .New() on here just to capture a stack trace.

But I can do that as part of my pr

ethanfrey · 2019-02-22T13:41:59Z

cmd/bnsd/x/nft/username/model.go

@@ -81,11 +81,11 @@ func (t *TokenDetails) Clone() *TokenDetails {

 func (t *TokenDetails) Validate() error {
 	if t == nil {
-		return errors.ErrInternalLegacy("must not be nil")
+		return errors.ErrInternal.New("must not be nil")
 	}


Token details must not be nil

ethanfrey · 2019-02-22T13:43:48Z

errors/common.go

-)
-
-// ABCI Response Codes
-// Base SDK reserves 0 ~ 99.


Delete looks good.

I was wondering if we just remove the entire main.go and common.go files, leaving just errors.go with the new code.

And move anything we need to errors.go to ensure all legacy code is gone

@ethanfrey I'd like to do this in separate iteration so as not to disturb your stack trace PR.

ethanfrey · 2019-02-22T13:45:34Z

errors/doc.go

@@ -0,0 +1,17 @@
+/**
+ Package errors implements custom error interfaces for weave.
+


Good start.

It would be nice to mention the importance of the abci error codes, as they are trasnmitted to the app and the only machine parse able response on an error

ethanfrey · 2019-02-22T13:47:19Z

x/batch/msg.go

@@ -23,7 +23,7 @@ func Validate(msg Msg) error {
 	}

 	if len(l) > MaxBatchMessages {
-		return errors.ErrTooLarge()
+		return errors.ErrInvalidInput.New("transaction is too large")


Can you add the number of message and the maximum limit here? This makes it easier to correct client side

ethanfrey · 2019-02-22T13:50:39Z

x/namecoin/handler.go

@@ -170,7 +170,7 @@ func (h SetNameHandler) Deliver(ctx weave.Context, db weave.KVStore,
 		return res, err
 	}
 	if obj == nil {
-		return res, ErrNoSuchWallet(msg.Address)
+		return res, errors.ErrNotFound.Newf("wallet %X", msg.Address)


Careful to use %X with weave.Address....

Since address.String will output as hex, using %X will hex encode the hex output and create confusion.

I just caught this one case, but please check the use of Address variables elsewhere also

@ethanfrey this is the same behavior we had before refactor. I can use string %s if that's easier.

actually, this is used all over the place in our code. I will change address usages to %s

ruseinov · 2019-02-22T16:13:28Z

@ethanfrey should be all done now.
I didn't touch error directory structure yet, let's merge both PRs and see what's obsolete.
I will add a ticket for myself to remove this - gonna be quick

ethanfrey

Good fixes

ruseinov added 3 commits February 20, 2019 19:07

blog errors eliminated

b20e2da

remove obsolete fields

4d04524

remove unused abci codes

1800d55

ruseinov requested review from alpe and ethanfrey February 20, 2019 18:08

ethanfrey reviewed Feb 21, 2019

View reviewed changes

ruseinov added 17 commits February 21, 2019 13:57

review comments

fe5a316

currency done

5198e97

more refactoring and legacy test removal

4c64d0b

fix currency tests

8a36752

fix tests

239d2bb

fix tests

c4a3aab

more common errors refactored

e49f08e

more refactoring

90748be

escrow done

96b1e14

multisig done

4b5a3f8

typo fixed

848684e

namecoin done

ff2aa5d

nft done

c3f5535

sigs done

381dd27

more error refactoring

4219ee5

doc

dd3080e

doc update

8a4a576

ruseinov changed the title ~~Feature/examples blog error refactor~~ Global error refactor and docs Feb 22, 2019

ethanfrey reviewed Feb 22, 2019

View reviewed changes

fix review comments

8f34244

newf

151e6a1

ethanfrey approved these changes Feb 22, 2019

View reviewed changes

ethanfrey merged commit 28a0cfc into master Feb 22, 2019

ethanfrey deleted the feature/examples-blog-error-refactor branch February 22, 2019 17:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Global error refactor and docs #339

Global error refactor and docs #339

ruseinov commented Feb 20, 2019 •

edited

Loading

codecov-io commented Feb 20, 2019 •

edited

Loading

ruseinov commented Feb 20, 2019

ethanfrey left a comment

ethanfrey Feb 21, 2019

ethanfrey Feb 21, 2019

ethanfrey Feb 21, 2019

ethanfrey Feb 21, 2019

ethanfrey Feb 21, 2019

ruseinov Feb 21, 2019

ethanfrey Feb 21, 2019

ruseinov Feb 21, 2019

ethanfrey left a comment

ethanfrey Feb 22, 2019

ethanfrey Feb 22, 2019

ethanfrey Feb 22, 2019

ethanfrey Feb 22, 2019

ethanfrey Feb 22, 2019

ethanfrey Feb 22, 2019

ruseinov Feb 22, 2019

ethanfrey Feb 22, 2019

ethanfrey Feb 22, 2019

ethanfrey Feb 22, 2019

ruseinov Feb 22, 2019

ruseinov Feb 22, 2019

ruseinov commented Feb 22, 2019

ethanfrey left a comment

		@@ -0,0 +1,17 @@
		/**
		Package errors implements custom error interfaces for weave.

Global error refactor and docs #339

Global error refactor and docs #339

Conversation

ruseinov commented Feb 20, 2019 • edited Loading

codecov-io commented Feb 20, 2019 • edited Loading

Codecov Report

ruseinov commented Feb 20, 2019

ethanfrey left a 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

ethanfrey left a 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

ruseinov commented Feb 22, 2019

ethanfrey left a comment

Choose a reason for hiding this comment

ruseinov commented Feb 20, 2019 •

edited

Loading

codecov-io commented Feb 20, 2019 •

edited

Loading