fix: use empty map instead of nil value for setting BaseApp.snapshotManager.extensions #12031

catShaark · 2022-05-24T17:24:04Z

Description

BaseApp method SetSnapshot sets BaseApp.snapshotManager.extensions with a nil value, this will cause nil map error whenever BaseApp.snapshotManager.extensions is used, so I change it to an empty map instead.

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

catShaark · 2022-05-24T17:32:30Z

Should I add unit test for this ?

yihuang

LGTM

yihuang · 2022-05-25T05:07:37Z

maybe better to change the NewManager internal to create a map if input with nil?

yihuang · 2022-05-25T05:10:07Z

https://github.com/CosmWasm/wasmd/blob/main/app/app.go#L744
wasmd has been using this feature, I wonder why it works for them?

tac0turtle · 2022-05-25T09:05:21Z

Should I add unit test for this ?

yes please

codecov · 2022-05-26T15:56:53Z

Codecov Report

Merging #12031 (1e95c65) into main (8c281ee) will decrease coverage by 0.35%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main   #12031      +/-   ##
==========================================
- Coverage   66.36%   66.00%   -0.36%     
==========================================
  Files         685      671      -14     
  Lines       71983    70987     -996     
==========================================
- Hits        47774    46858     -916     
+ Misses      21518    21467      -51     
+ Partials     2691     2662      -29

Impacted Files	Coverage Δ
snapshots/manager.go	`47.58% <100.00%> (+0.54%)`	⬆️
x/distribution/simulation/operations.go	`80.21% <0.00%> (-9.90%)`	⬇️
depinject/one_per_module.go
depinject/supply.go
depinject/module_key.go
depinject/struct_args.go
depinject/location.go
depinject/errors.go
depinject/inject.go
depinject/debug.go
... and 6 more

faddat · 2022-05-26T16:07:39Z

@yihuang honestly, I don't know -- I tried to implement line for line the same way as they did in the current wasmd. This PR is actually for wasmd.

Please feel free to comment / let us know if you see the issue?

CosmWasm/wasmd#863

Thanks very much @yihuang <3

catShaark · 2022-05-26T18:14:36Z

https://github.com/CosmWasm/wasmd/blob/main/app/app.go#L744 wasmd has been using this feature, I wonder why it works for them?

I think it's because they don't call the SetSnapshot func to set BaseApp.snapshotManager, which makes manager == nil , and therefore this line is never run

alexanderbez · 2022-05-26T19:40:39Z

Once we get the liveness test figure out and working on main, we'll merge this 👍

yihuang · 2022-05-27T01:41:41Z

@yihuang honestly, I don't know -- I tried to implement line for line the same way as they did in the current wasmd. This PR is actually for wasmd.

Please feel free to comment / let us know if you see the issue?

CosmWasm/wasmd#863

Thanks very much @yihuang <3

811cb8a#diff-84a22ba89cc7868bbafc59e07c35ba0bffb05115962cb885bf39eef1a1a8a66eR68

The v0.45.x backported version has a slight difference to avoid breaking the existing API, lol.

amaury1093 · 2022-06-01T14:44:03Z

Is this ready to be merged?

This branch is out-of-date with the base branch

@catShaark Can you merge / rebase main to this branch? Or allow edits from maintainers

catShaark · 2022-06-01T15:29:43Z

@yihuang , I've updated this branch

yihuang · 2022-06-01T15:53:34Z

@yihuang , I've updated this branch

LGTM

…anager.extensions (cosmos#12031) ## Description `BaseApp` method `SetSnapshot` sets `BaseApp.snapshotManager.extensions` with a nil value, this will cause nil map error whenever `BaseApp.snapshotManager.extensions` is used, so I change it to an empty map instead. ### Author Checklist *All items are required. Please add a note to the item if the item is not applicable and please add links to any relevant follow up issues.* I have... - [x] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [x] added `!` to the type prefix if API or client breaking change - [x] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#pr-targeting)) - [x] provided a link to the relevant issue or specification - [x] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/main/docs/building-modules) - [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#testing) - [ ] added a changelog entry to `CHANGELOG.md` - [x] included comments for [documenting Go code](https://blog.golang.org/godoc) - [x] updated the relevant documentation or specification - [x] reviewed "Files changed" and left comments if necessary - [x] confirmed all CI checks have passed ### Reviewers Checklist *All items are required. Please add a note if the item is not applicable and please add your handle next to the items reviewed if you only reviewed selected items.* I have... - [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] confirmed `!` in the type prefix if API or client breaking change - [ ] confirmed all author checklist items have been addressed - [ ] reviewed state machine logic - [ ] reviewed API design and naming - [ ] reviewed documentation is accurate - [ ] reviewed tests and test coverage - [ ] manually tested (if applicable)

…anager.extensions (#12031) (#12458)

…anager.extensions (cosmos#12031) ## Description `BaseApp` method `SetSnapshot` sets `BaseApp.snapshotManager.extensions` with a nil value, this will cause nil map error whenever `BaseApp.snapshotManager.extensions` is used, so I change it to an empty map instead. ### Author Checklist *All items are required. Please add a note to the item if the item is not applicable and please add links to any relevant follow up issues.* I have... - [x] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [x] added `!` to the type prefix if API or client breaking change - [x] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#pr-targeting)) - [x] provided a link to the relevant issue or specification - [x] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/main/docs/building-modules) - [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#testing) - [ ] added a changelog entry to `CHANGELOG.md` - [x] included comments for [documenting Go code](https://blog.golang.org/godoc) - [x] updated the relevant documentation or specification - [x] reviewed "Files changed" and left comments if necessary - [x] confirmed all CI checks have passed ### Reviewers Checklist *All items are required. Please add a note if the item is not applicable and please add your handle next to the items reviewed if you only reviewed selected items.* I have... - [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] confirmed `!` in the type prefix if API or client breaking change - [ ] confirmed all author checklist items have been addressed - [ ] reviewed state machine logic - [ ] reviewed API design and naming - [ ] reviewed documentation is accurate - [ ] reviewed tests and test coverage - [ ] manually tested (if applicable)

catShaark requested a review from a team as a code owner May 24, 2022 17:24

catShaark changed the title ~~use empty map instead of nil for setting snapshotManager.extensions~~ fix: use empty map instead of nil for setting BaseApp.snapshotManager.extensions May 24, 2022

catShaark changed the title ~~fix: use empty map instead of nil for setting BaseApp.snapshotManager.extensions~~ fix: nil value setting BaseApp.snapshotManager.extensions May 24, 2022

catShaark changed the title ~~fix: nil value setting BaseApp.snapshotManager.extensions~~ fix: use empty map instead of nil value for setting BaseApp.snapshotManager.extensions May 24, 2022

yihuang approved these changes May 25, 2022

View reviewed changes

catShaark and others added 3 commits May 25, 2022 07:14

use empty map instead of nil for setting snapshotManager.extensions

9e25c41

Merge branch 'main' into fix-baseapp-SetSnapshot

83ad311

Merge branch 'main' into fix-baseapp-SetSnapshot

848e868

catShaark and others added 2 commits May 25, 2022 17:11

Merge branch 'main' into fix-baseapp-SetSnapshot

8b3167e

change snapshots.NewManager()

b027cf0

alexanderbez approved these changes May 26, 2022

View reviewed changes

Merge branch 'main' into fix-baseapp-SetSnapshot

c9f07e8

faddat mentioned this pull request May 26, 2022

Cosmos SDK v46 CosmWasm/wasmd#863

Closed

Merge branch 'main' into fix-baseapp-SetSnapshot

a4cf8ab

ValarDragon approved these changes May 27, 2022

View reviewed changes

catShaark added 2 commits May 31, 2022 16:53

Merge branch 'main' into fix-baseapp-SetSnapshot

0e06609

Merge branch 'main' into fix-baseapp-SetSnapshot

0d1b990

Merge branch 'main' into fix-baseapp-SetSnapshot

1935ae1

amaury1093 added the A:automerge Automatically merge PR once all prerequisites pass. label Jun 1, 2022

vuong177 and others added 2 commits June 2, 2022 03:46

Merge branch 'main' into fix-baseapp-SetSnapshot

a5e58f3

Merge branch 'main' into fix-baseapp-SetSnapshot

1e95c65

mergify bot merged commit 93f5108 into cosmos:main Jun 2, 2022

alexanderbez pushed a commit that referenced this pull request Jul 5, 2022

fix: use empty map instead of nil value for setting BaseApp.snapshotM…

c7c6588

…anager.extensions (#12031) (#12458)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: use empty map instead of nil value for setting BaseApp.snapshotManager.extensions #12031

fix: use empty map instead of nil value for setting BaseApp.snapshotManager.extensions #12031

catShaark commented May 24, 2022 •

edited

Loading

catShaark commented May 24, 2022

yihuang left a comment

yihuang commented May 25, 2022

yihuang commented May 25, 2022

tac0turtle commented May 25, 2022

codecov bot commented May 26, 2022 •

edited

Loading

faddat commented May 26, 2022 •

edited

Loading

catShaark commented May 26, 2022 •

edited

Loading

alexanderbez commented May 26, 2022

yihuang commented May 27, 2022

amaury1093 commented Jun 1, 2022 •

edited

Loading

catShaark commented Jun 1, 2022

yihuang commented Jun 1, 2022

fix: use empty map instead of nil value for setting BaseApp.snapshotManager.extensions #12031

fix: use empty map instead of nil value for setting BaseApp.snapshotManager.extensions #12031

Conversation

catShaark commented May 24, 2022 • edited Loading

Description

Author Checklist

Reviewers Checklist

catShaark commented May 24, 2022

yihuang left a comment

Choose a reason for hiding this comment

yihuang commented May 25, 2022

yihuang commented May 25, 2022

tac0turtle commented May 25, 2022

codecov bot commented May 26, 2022 • edited Loading

Codecov Report

faddat commented May 26, 2022 • edited Loading

catShaark commented May 26, 2022 • edited Loading

alexanderbez commented May 26, 2022

yihuang commented May 27, 2022

amaury1093 commented Jun 1, 2022 • edited Loading

catShaark commented Jun 1, 2022

yihuang commented Jun 1, 2022

catShaark commented May 24, 2022 •

edited

Loading

codecov bot commented May 26, 2022 •

edited

Loading

faddat commented May 26, 2022 •

edited

Loading

catShaark commented May 26, 2022 •

edited

Loading

amaury1093 commented Jun 1, 2022 •

edited

Loading