fix: Prevent newExporter crash if tree.ndb is nil #622
Conversation
export.go
Outdated
| } | ||
| // CV Prevent crash on incrVersionReaders if tree.ndb == nil | ||
| if tree.ndb == nil { | ||
| fmt.Printf("iavl/export newExporter failed to create because tree.ndb is nil\n") |
There was a problem hiding this comment.
this function should return error. Doing fmt.Printf doesn't make sense here.
There was a problem hiding this comment.
Could just return (nil, err) with fmt.Errorf to start with. Then any more complicated error handling can be followed up in other commits/improvment?
Or is there some error standard you're trying to maintain?
There was a problem hiding this comment.
yea that makes sense, we would then handle this in the application
There was a problem hiding this comment.
Returning error now, updated api/test
|
Last commit returns explicit error as recommended. |
export.go
Outdated
| } | ||
| // CV Prevent crash on incrVersionReaders if tree.ndb == nil | ||
| if tree.ndb == nil { | ||
| return nil, ErrorTreeNodeDbNil |
There was a problem hiding this comment.
maybe we can have one error type: ErrNotIniitalizedTree. And then use wrapping to add more details, eg:
ErrNotIniitalizedTree.Wrap("ndb is nil")
|
change log updated, errors wrapped |
|
note change in errors package |
export.go
Outdated
| func newExporter(tree *ImmutableTree) *Exporter { | ||
| func newExporter(tree *ImmutableTree) (*Exporter, error) { | ||
| if tree == nil { | ||
| return nil, errors.Wrap(ErrNotInitalizedTree, "tree is nil") |
There was a problem hiding this comment.
can we use fmt.Errorf to avoid introducing GitHub.com/pkg/errors as a dependency
There was a problem hiding this comment.
My mistake. I thought the team was looking to call Wrap() specifically. Will change it again.
There was a problem hiding this comment.
no worries, pkg/errors is deprecated with go 1.18 and beyond because of fmt.errorf and/or errors pkg.
There was a problem hiding this comment.
Learn something new every day
|
Updated : change from requested Wrap to fmt.Errorf |
tac0turtle
changed the title
|
linting and tests are failing, could you fix and we will handle backport |
Will give it a shot and let you know if help is required :) |
|
Issue was hiding in the benchmarks, fixed and running through tests. |
|
looks like there's some fumpting to do (sorry not yet completely familiar with all the tests in this repo) |
|
went fumpting |
|
Also as a note, noticed: But in Makefile it still tries to build iavlserver Want it removed? |
|
Removed failing build of cmd/iavlserver. Let me know if you want it back. |
|
thanks for the pr @chillyvee |
|
@Mergifyio backport release/v0.19.x |
|
@Mergifyio backport release/v0.19.x |
✅ Backports have been created
|
Co-authored-by: Marko <marbar3778@yahoo.com> (cherry picked from commit 2777659) # Conflicts: # export.go
…osmos#622) (cosmos#631)" This reverts commit 2af006a.
…osmos#622) (cosmos#631)" This reverts commit 2af006a.
… (cosmos#631) Co-authored-by: Chill Validation <92176880+chillyvee@users.noreply.github.com> Co-authored-by: Marko Baricevic <marbar3778@yahoo.com>
v0.19.5 + cosmos/iavl#622
Found a case where dig chain did not have all stores configured with a nodedb (for whatever reason)
It is better to return an error so that cosmos-sdk can determine whether to skip the misconfigured store or panic.
Would also like to backport to fix v0.19.4 (but need assistance to target the right branch)