feat: Make extension snapshotter interface safer to use #11825

yihuang · 2022-04-29T01:44:41Z

Closes: #11824
Solution:

Use new methods SnapshotExtension/RestoreExtension to handle payload stream specifically.
Improve unit tests.

Description

more details in the closed issue.

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)

Closes: cosmos#11824 Solution: - Use new methods `SnapshotExtension`/`RestoreExtension` to handle payload stream specifically. - Improve unit tests.

CHANGELOG.md

yihuang · 2022-04-29T01:53:44Z

snapshots/helpers_test.go

+	return nil
+}
+
+func (s *extSnapshotter) RestoreExtension(height uint64, format uint32, payloadReader snapshottypes.ExtensionPayloadReader) error {


An typical extension implementation.

github-actions · 2022-06-17T00:04:46Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

tac0turtle · 2022-06-22T08:23:11Z

@JeancarloBarrios any chance you can review this, seems like this was assigned with no follow wup

tac0turtle

@alexanderbez could you pop in for a review

codecov · 2022-08-15T14:08:58Z

Codecov Report

Merging #11825 (bba13cf) into main (0ed7360) will increase coverage by 0.07%.
The diff coverage is 57.14%.

@@            Coverage Diff             @@
##             main   #11825      +/-   ##
==========================================
+ Coverage   55.50%   55.57%   +0.07%     
==========================================
  Files         646      646              
  Lines       54870    54891      +21     
==========================================
+ Hits        30453    30508      +55     
+ Misses      21962    21919      -43     
- Partials     2455     2464       +9

Impacted Files	Coverage Δ
snapshots/manager.go	`56.91% <57.14%> (+9.32%)`	⬆️
crypto/keys/internal/ecdsa/privkey.go	`81.13% <0.00%> (-1.89%)`	⬇️
x/group/keeper/keeper.go	`55.90% <0.00%> (-0.40%)`	⬇️
x/distribution/simulation/operations.go	`90.42% <0.00%> (+9.57%)`	⬆️

snapshots/types/util.go

tac0turtle · 2022-08-16T06:01:10Z

snapshots/helpers_test.go

 	m.items = [][]byte{}
 	for {
-		item := &snapshottypes.SnapshotItem{}
-		err := protoReader.ReadMsg(item)
+		item.Reset()


is there a test case to test this in handled correctly? I recently ran into an issue where we used .Reset like this and the api is different than what we expected

func (m *SnapshotItem) Reset() { *m = SnapshotItem{} }

looks straightforward enough to me, what issue did you have? Do you have a concern that after ReadMsg, some fields still keep the values from the last one?

snapshots/types/snapshotter.go

snapshots/types/util.go

CHANGELOG.md

alexanderbez · 2022-08-17T15:19:53Z

@yihuang could you quickly take a look at the linting failures pls?

types/events.go:6: File is not `gofumpt`-ed (gofumpt)
	"golang.org/x/exp/maps"
	"golang.org/x/exp/slices"
types/events.go:10: File is not `gofumpt`-ed (gofumpt)
	"strings"

yihuang · 2022-08-18T01:31:54Z

@yihuang could you quickly take a look at the linting failures pls?

types/events.go:6: File is not `gofumpt`-ed (gofumpt)
	"golang.org/x/exp/maps"
	"golang.org/x/exp/slices"
types/events.go:10: File is not `gofumpt`-ed (gofumpt)
	"strings"

It's strange this PR doesn't touch this file. I update the main branch again and it disppears.

* Make extension snapshotter interface safer to use Closes: cosmos#11824 Solution: - Use new methods `SnapshotExtension`/`RestoreExtension` to handle payload stream specifically. - Improve unit tests. * update changelog * Update snapshots/types/util.go * changelog * go linter * Update CHANGELOG.md Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>

* Make extension snapshotter interface safer to use Closes: cosmos#11824 Solution: - Use new methods `SnapshotExtension`/`RestoreExtension` to handle payload stream specifically. - Improve unit tests. * update changelog * Update snapshots/types/util.go * changelog * go linter * Update CHANGELOG.md Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com> (cherry picked from commit e397434)

Make extension snapshotter interface safer to use

f8ee15d

Closes: cosmos#11824 Solution: - Use new methods `SnapshotExtension`/`RestoreExtension` to handle payload stream specifically. - Improve unit tests.

yihuang requested review from aaronc and alexanderbez as code owners April 29, 2022 01:44

github-actions bot added the T: ADR An issue or PR relating to an architectural decision record label Apr 29, 2022

yihuang commented Apr 29, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

update changelog

c659954

yihuang mentioned this pull request Apr 29, 2022

Add state sync support CosmWasm/wasmd#823

Merged

yihuang commented Apr 29, 2022

View reviewed changes

Merge branch 'main' into safer-snapshotter-api

ce63f95

yihuang requested a review from a team as a code owner April 30, 2022 16:37

amaury1093 assigned amaury1093 and JeancarloBarrios May 2, 2022

github-actions bot added the stale label Jun 17, 2022

github-actions bot removed the stale label Jun 23, 2022

tac0turtle unassigned amaury1093 and JeancarloBarrios Jul 11, 2022

tac0turtle reviewed Aug 15, 2022

View reviewed changes

Merge branch 'main' into safer-snapshotter-api

3c2ff2b

github-actions bot added the Type: ADR label Aug 15, 2022

yihuang commented Aug 15, 2022

View reviewed changes

snapshots/types/util.go Outdated Show resolved Hide resolved

yihuang added 2 commits August 15, 2022 22:13

Update snapshots/types/util.go

9579e89

changelog

9c69a3e

tac0turtle requested a review from ethanfrey August 16, 2022 05:59

tac0turtle reviewed Aug 16, 2022

View reviewed changes

yihuang commented Aug 16, 2022

View reviewed changes

snapshots/types/snapshotter.go Outdated Show resolved Hide resolved

go linter

87a15ad

alexanderbez approved these changes Aug 16, 2022

View reviewed changes

snapshots/types/util.go Show resolved Hide resolved

yihuang commented Aug 17, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

Update CHANGELOG.md

16231ea

alexanderbez changed the title ~~Make extension snapshotter interface safer to use~~ feat: Make extension snapshotter interface safer to use Aug 17, 2022

alexanderbez added 3 commits August 17, 2022 10:52

Merge branch 'main' into safer-snapshotter-api

008f6b7

Merge branch 'main' into safer-snapshotter-api

14be33a

Merge branch 'main' into safer-snapshotter-api

705e198

Merge branch 'main' into safer-snapshotter-api

bba13cf

tac0turtle merged commit e397434 into cosmos:main Aug 18, 2022

yihuang deleted the safer-snapshotter-api branch August 18, 2022 07:35

ryanchristo mentioned this pull request Dec 16, 2022

fix: gov and group amino and snapshot hashes regen-network/cosmos-sdk#49

Merged

19 tasks

coderabbitai bot mentioned this pull request Apr 18, 2024

feat: explicit config file entry for ABCI client type #20080

Closed

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Make extension snapshotter interface safer to use #11825

feat: Make extension snapshotter interface safer to use #11825

yihuang commented Apr 29, 2022 •

edited

Loading

yihuang Apr 29, 2022 •

edited

Loading

github-actions bot commented Jun 17, 2022

tac0turtle commented Jun 22, 2022

tac0turtle left a comment

codecov bot commented Aug 15, 2022 •

edited

Loading

tac0turtle Aug 16, 2022

yihuang Aug 16, 2022 •

edited

Loading

alexanderbez commented Aug 17, 2022

yihuang commented Aug 18, 2022

feat: Make extension snapshotter interface safer to use #11825

feat: Make extension snapshotter interface safer to use #11825

Conversation

yihuang commented Apr 29, 2022 • edited Loading

Description

Author Checklist

Reviewers Checklist

yihuang Apr 29, 2022 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Jun 17, 2022

tac0turtle commented Jun 22, 2022

tac0turtle left a comment

Choose a reason for hiding this comment

codecov bot commented Aug 15, 2022 • edited Loading

Codecov Report

tac0turtle Aug 16, 2022

Choose a reason for hiding this comment

yihuang Aug 16, 2022 • edited Loading

Choose a reason for hiding this comment

alexanderbez commented Aug 17, 2022

yihuang commented Aug 18, 2022

yihuang commented Apr 29, 2022 •

edited

Loading

yihuang Apr 29, 2022 •

edited

Loading

codecov bot commented Aug 15, 2022 •

edited

Loading

yihuang Aug 16, 2022 •

edited

Loading