Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix IAVL Store Commit #4880

Merged
merged 6 commits into from
Aug 11, 2019
Merged

Fix IAVL Store Commit #4880

merged 6 commits into from
Aug 11, 2019

Conversation

alexanderbez
Copy link
Contributor

@alexanderbez alexanderbez commented Aug 9, 2019

MutableTree#DeleteVersion only returns standard errors now.


  • 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.

  • Wrote tests

  • Updated relevant documentation (docs/)

  • Added a relevant changelog entry: clog add [section] [-t <tag>] [-m <msg>]

  • Re-reviewed Files changed in the github PR explorer


For Admin Use:

  • Added appropriate labels to PR (ex. wip, ready-for-review, docs)
  • Reviewers Assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@alexanderbez alexanderbez requested a review from tac0turtle August 9, 2019 18:10
@codecov
Copy link

codecov bot commented Aug 9, 2019

Codecov Report

Merging #4880 into master will not change coverage.
The diff coverage is 0%.

@@           Coverage Diff           @@
##           master    #4880   +/-   ##
=======================================
  Coverage   54.05%   54.05%           
=======================================
  Files         269      269           
  Lines       17116    17116           
=======================================
  Hits         9252     9252           
  Misses       7178     7178           
  Partials      686      686

@ethanfrey
Copy link
Contributor

I think it is best to keep the same logic (and add a test).

You rightly noted that this broke as the iavl error format changes:

	if _, ok := tree.versions[version]; !ok {
		return errors.Wrap(ErrVersionDoesNotExist, "")
	}

https://github.com/tendermint/iavl/blob/master/mutable_tree.go#L436-L438

This can be detected (safely) with errors.Cause(err) == iavl.ErrVersionDoesNotExist.

I would propose simply changing the original line:
if err != nil && err.(cmn.Error).Data() != iavl.ErrVersionDoesNotExist {
to
if err != nil && errors.Cause(err) != iavl.ErrVersionDoesNotExist {

That seems the cleaner fix (and maybe a test there)

@alexanderbez alexanderbez merged commit a6e776c into master Aug 11, 2019
@alexanderbez alexanderbez deleted the bez/fix-iavl-store-commit-error branch August 11, 2019 22:46
alexanderbez added a commit that referenced this pull request Aug 13, 2019
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants