From 805f653d0fb84dadb242170646f7bd7c4a3e330b Mon Sep 17 00:00:00 2001
From: Adu <foriteration@gmail.com>
Date: Wed, 19 Jan 2022 21:12:32 +0800
Subject: [PATCH 01/12] docs: ADR-049 state sync hooks

---
 docs/architecture/adr-049-state-sync-hooks.md | 161 ++++++++++++++++++
 1 file changed, 161 insertions(+)
 create mode 100644 docs/architecture/adr-049-state-sync-hooks.md

diff --git a/docs/architecture/adr-049-state-sync-hooks.md b/docs/architecture/adr-049-state-sync-hooks.md
new file mode 100644
index 000000000000..23b2defab7ff
--- /dev/null
+++ b/docs/architecture/adr-049-state-sync-hooks.md
@@ -0,0 +1,161 @@
+# ADR 049: State Sync Hooks
+
+## Changelog
+
+- Jan 19, 2022: Initial Draft
+
+## Status
+
+Draft, Under Implementation
+
+## Abstract
+
+This ADR provides hooks for app modules to publish additional state(outside of IAVL tree) for state-sync.
+
+## Context
+
+New clients uses state-sync to download snapshots of module state from peer nodes. Currently, the snapshot consists of a
+stream of `SnapshotStoreItem` and `SnapshotIAVLItem`, which means for app modules that maintain their states outside of
+the IAVL tree, they can not add their states to the snapshot stream for state-sync.
+
+## Decision
+
+A simple backward-compatible proposal based on our existing implementation is that, we can add two new message types:
+`SnapshotExtensionMeta` and `SnapshotExtensionPayload`, and they are appended to the existing multi-store stream with `SnapshotExtensionMeta` acting as a delimiter between modules.
+
+```proto
+// SnapshotItem is an item contained in a rootmulti.Store snapshot.
+message SnapshotItem {
+  // item is the specific type of snapshot item.
+  oneof item {
+    SnapshotStoreItem        store             = 1;
+    SnapshotIAVLItem         iavl              = 2 [(gogoproto.customname) = "IAVL"];
+    SnapshotExtensionMeta    extension         = 3;
+    SnapshotExtensionPayload extension_payload = 4;
+  }
+}
+
+// SnapshotStoreItem contains metadata about a snapshotted store.
+message SnapshotStoreItem {
+  string name = 1;
+}
+
+// SnapshotIAVLItem is an exported IAVL node.
+message SnapshotIAVLItem {
+  bytes key     = 1;
+  bytes value   = 2;
+  int64 version = 3;
+  int32 height  = 4;
+}
+
+// SnapshotExtensionMeta contains metadata about an external snapshotter.
+message SnapshotExtensionMeta {
+  string name   = 1;
+  uint32 format = 2;
+}
+
+// SnapshotExtensionPayload contains payloads of an external snapshotter.
+message SnapshotExtensionPayload {
+  bytes payload = 1;
+}
+```
+
+The snapshot stream would look like this:
+
+```go
+// multi-store snapshot
+{SnapshotStoreItem | SnapshotIAVLItem, ...}
+// module1 snapshot
+SnapshotExtensionMeta
+{SnapshotExtensionPayload, ...}
+// module2 snapshot
+SnapshotExtensionMeta
+{SnapshotExtensionPayload, ...}
+```
+
+Add `extensions` field to snapshot `Manager` for extension snapshotters. Multistore snapshotter is a special one and it doesn't need a name because it is always placed at the beginning of the binary stream.
+
+```go
+type Manager struct {
+	store      *Store
+	multistore types.Snapshotter
+	extensions map[string]types.NamedSnapshotter
+    mtx                sync.Mutex
+	operation          operation
+	chRestore          chan<- io.ReadCloser
+	chRestoreDone      <-chan restoreDone
+	restoreChunkHashes [][]byte
+	restoreChunkIndex  uint32
+}
+```
+
+As with `Snapshotter` interface for `multistore` snapshotter, two more function signatures: `SnapshotFormat` and `SupportedFormats` are added.
+<ul>
+<li> removal of format parameter: `Snapshotter` chooses a format autonomously, not pass in from the caller. </li>
+<li> `Snapshotter` announces the format it uses to snapshot with method `SnapshotFormat()`. </li>
+<ul>
+
+```go
+type Snapshotter interface {
+	// Snapshot writes snapshot items into the protobuf writer.
+	Snapshot(height uint64, protoWriter protoio.WriteCloser) error
+
+	// Restore restores a state snapshot from the protobuf items read from the reader.
+	// If the ready channel is non-nil, it returns a ready signal (by being closed) once the
+	// restorer is ready to accept chunks.
+	Restore(height uint64, format uint32, protoReader protoio.ReadCloser) (SnapshotItem, error)
+
+	// SnapshotFormat returns the default format used to take a snapshot.
+	SnapshotFormat() uint32
+
+	// SupportedFormats returns a list of formats it can restore from.
+	SupportedFormats() []uint32
+}
+```
+
+Add `NamedSnapshotter` interface for extension snapshotters.
+
+```go
+type NamedSnapshotter interface {
+	Snapshotter
+
+	// SnapshotterName returns the name of snapshotter, it should be unique in the manager.
+	SnapshotterName() string
+}
+```
+
+## Consequences
+
+As a result of this implementation, we are able to create snapshots of chunk binary stream for the state that we maintain outside of the IAVL Tree, CosmWasm for example. And new clients are able to fetch sanpshots of state for all modules that have implemented the corresponding interface from peer nodes. 
+
+
+### Backwards Compatibility
+
+This ADR introduces new proto message types, add field for extension snapshotters to snapshot `Manager`, add new function signatures `SnapshotFormat()`, `SupportedFormats()` to `Snapshotter` interface and add new `NamedSnapshotter` interface, so this is not backwards compatible.
+
+### Positive
+
+State maintained outside of IAVL tree like CosmWasm blobs can create snapshots by implementing extension snapshotters, and being fetched by new clients via state-sync.
+
+### Negative
+
+// Todo
+
+### Neutral
+
+All modules that maintain state outside of IAVL tree need to implement `NamedSnapshotter` and the snapshot manager need to call `RegisterExtensions` when setting up the application.
+
+## Further Discussions
+
+While an ADR is in the DRAFT or PROPOSED stage, this section should contain a summary of issues to be solved in future iterations (usually referencing comments from a pull-request discussion).
+Later, this section can optionally list ideas or improvements the author or reviewers found during the analysis of this ADR.
+
+## Test Cases [optional]
+
+Test cases for an implementation are mandatory for ADRs that are affecting consensus changes. Other ADRs can choose to include links to test cases if applicable.
+
+## References
+
+- https://github.com/cosmos/cosmos-sdk/pull/10961
+- https://github.com/cosmos/cosmos-sdk/issues/7340
+- https://hackmd.io/gJoyev6DSmqqkO667WQlGw

From 8eb5147a779800058f98bc865871e64b162bc08d Mon Sep 17 00:00:00 2001
From: Adu <foriteration@gmail.com>
Date: Thu, 20 Jan 2022 09:47:10 +0800
Subject: [PATCH 02/12] fix some obscure expressions and add RegisterExtensions
 part

---
 docs/architecture/adr-049-state-sync-hooks.md | 25 +++++++++++++------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/docs/architecture/adr-049-state-sync-hooks.md b/docs/architecture/adr-049-state-sync-hooks.md
index 23b2defab7ff..5abf78eb5401 100644
--- a/docs/architecture/adr-049-state-sync-hooks.md
+++ b/docs/architecture/adr-049-state-sync-hooks.md
@@ -10,18 +10,19 @@ Draft, Under Implementation
 
 ## Abstract
 
-This ADR provides hooks for app modules to publish additional state(outside of IAVL tree) for state-sync.
+This ADR provides hooks for app modules to publish snapshot of additional state(outside of IAVL tree) for state-sync.
 
 ## Context
 
-New clients uses state-sync to download snapshots of module state from peer nodes. Currently, the snapshot consists of a
+New clients use state-sync to download snapshots of module state from peer nodes. Currently, the snapshot consists of a
 stream of `SnapshotStoreItem` and `SnapshotIAVLItem`, which means for app modules that maintain their states outside of
 the IAVL tree, they can not add their states to the snapshot stream for state-sync.
 
 ## Decision
 
-A simple backward-compatible proposal based on our existing implementation is that, we can add two new message types:
-`SnapshotExtensionMeta` and `SnapshotExtensionPayload`, and they are appended to the existing multi-store stream with `SnapshotExtensionMeta` acting as a delimiter between modules.
+A simple proposal based on our existing implementation is that, we can add two new message types: `SnapshotExtensionMeta` 
+and `SnapshotExtensionPayload`, and they are appended to the existing multi-store stream with `SnapshotExtensionMeta` 
+acting as a delimiter between extensions.
 
 ```proto
 // SnapshotItem is an item contained in a rootmulti.Store snapshot.
@@ -65,10 +66,10 @@ The snapshot stream would look like this:
 ```go
 // multi-store snapshot
 {SnapshotStoreItem | SnapshotIAVLItem, ...}
-// module1 snapshot
+// extension1 snapshot
 SnapshotExtensionMeta
 {SnapshotExtensionPayload, ...}
-// module2 snapshot
+// extension2 snapshot
 SnapshotExtensionMeta
 {SnapshotExtensionPayload, ...}
 ```
@@ -89,7 +90,15 @@ type Manager struct {
 }
 ```
 
-As with `Snapshotter` interface for `multistore` snapshotter, two more function signatures: `SnapshotFormat` and `SupportedFormats` are added.
+For extension snapshotters that have implemented the `NamedSnapshotter` interface, their names should be registered to the snapshotter `Manager` by calling 
+`RegisterExtensions` when setting up the application.
+
+```go
+// RegisterExtensions register extension snapshotters to manager
+func (m *Manager) RegisterExtensions(extensions ...types.NamedSnapshotter) error 
+```
+
+As with `Snapshotter` interface for `multistore` snapshotter, two more function signatures: `SnapshotFormat()` and `SupportedFormats()` are added.
 <ul>
 <li> removal of format parameter: `Snapshotter` chooses a format autonomously, not pass in from the caller. </li>
 <li> `Snapshotter` announces the format it uses to snapshot with method `SnapshotFormat()`. </li>
@@ -126,7 +135,7 @@ type NamedSnapshotter interface {
 
 ## Consequences
 
-As a result of this implementation, we are able to create snapshots of chunk binary stream for the state that we maintain outside of the IAVL Tree, CosmWasm for example. And new clients are able to fetch sanpshots of state for all modules that have implemented the corresponding interface from peer nodes. 
+As a result of this implementation, we are able to create snapshots of binary chunk stream for the state that we maintain outside of the IAVL Tree, CosmWasm blobs for example. And new clients are able to fetch sanpshots of state for all modules that have implemented the corresponding interface from peer nodes. 
 
 
 ### Backwards Compatibility

From 3b52898cc148f2e5edb3f4fd40b67bb373e4c839 Mon Sep 17 00:00:00 2001
From: Adu <foriteration@gmail.com>
Date: Fri, 21 Jan 2022 12:04:26 +0800
Subject: [PATCH 03/12] add explanations to clarify some details

---
 docs/architecture/adr-049-state-sync-hooks.md | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/docs/architecture/adr-049-state-sync-hooks.md b/docs/architecture/adr-049-state-sync-hooks.md
index 5abf78eb5401..9545e78ead90 100644
--- a/docs/architecture/adr-049-state-sync-hooks.md
+++ b/docs/architecture/adr-049-state-sync-hooks.md
@@ -22,7 +22,13 @@ the IAVL tree, they can not add their states to the snapshot stream for state-sy
 
 A simple proposal based on our existing implementation is that, we can add two new message types: `SnapshotExtensionMeta` 
 and `SnapshotExtensionPayload`, and they are appended to the existing multi-store stream with `SnapshotExtensionMeta` 
-acting as a delimiter between extensions.
+acting as a delimiter between extensions. As the chunk hashes should be able to ensure data integrity, we don't need a
+delimiter to mark the end of the snapshot stream.
+
+Besides, we provide `Snapshotter` and `NamedSnapshotter` interface for modules to implement snapshotters, which will handle both taking 
+snapshot and the restoration. Each module could have mutiple snapshotters, and for modules with additional state, they should
+implement `NamedSnapshotter` as extension snapshotters. When setting up the application, the snapshot `Manager` should call 
+`RegisterExtensions([]NamedSnapshotter…)` to register all the extension snapshotters.
 
 ```proto
 // SnapshotItem is an item contained in a rootmulti.Store snapshot.
@@ -50,8 +56,10 @@ message SnapshotIAVLItem {
 }
 
 // SnapshotExtensionMeta contains metadata about an external snapshotter.
+// One module may need multiple snapshotters, so each module may have multiple SnapshotExtensionMeta.
 message SnapshotExtensionMeta {
   string name   = 1;
+  // format is used within the snapshotter/namespace, not global one for all modules
   uint32 format = 2;
 }
 
@@ -90,8 +98,8 @@ type Manager struct {
 }
 ```
 
-For extension snapshotters that have implemented the `NamedSnapshotter` interface, their names should be registered to the snapshotter `Manager` by calling 
-`RegisterExtensions` when setting up the application.
+For extension snapshotters that implement the `NamedSnapshotter` interface, their names should be registered to the snapshot `Manager` by 
+calling `RegisterExtensions` when setting up the application. And the snapshotters will handle both taking snapshot and restoration.
 
 ```go
 // RegisterExtensions register extension snapshotters to manager
@@ -152,7 +160,7 @@ State maintained outside of IAVL tree like CosmWasm blobs can create snapshots b
 
 ### Neutral
 
-All modules that maintain state outside of IAVL tree need to implement `NamedSnapshotter` and the snapshot manager need to call `RegisterExtensions` when setting up the application.
+All modules that maintain state outside of IAVL tree need to implement `NamedSnapshotter` and the snapshot `Manager` need to call `RegisterExtensions` when setting up the application.
 
 ## Further Discussions
 

From 4d05187f966362ffddf53c14dc7cdff1878c8128 Mon Sep 17 00:00:00 2001
From: Adu <foriteration@gmail.com>
Date: Tue, 25 Jan 2022 11:09:57 +0800
Subject: [PATCH 04/12] change NamedSnapshotter to ExtensionSnapshotter based
 on the implementation

---
 docs/architecture/adr-049-state-sync-hooks.md | 52 +++++++++++--------
 1 file changed, 29 insertions(+), 23 deletions(-)

diff --git a/docs/architecture/adr-049-state-sync-hooks.md b/docs/architecture/adr-049-state-sync-hooks.md
index 9545e78ead90..6cce9b659609 100644
--- a/docs/architecture/adr-049-state-sync-hooks.md
+++ b/docs/architecture/adr-049-state-sync-hooks.md
@@ -25,10 +25,10 @@ and `SnapshotExtensionPayload`, and they are appended to the existing multi-stor
 acting as a delimiter between extensions. As the chunk hashes should be able to ensure data integrity, we don't need a
 delimiter to mark the end of the snapshot stream.
 
-Besides, we provide `Snapshotter` and `NamedSnapshotter` interface for modules to implement snapshotters, which will handle both taking 
+Besides, we provide `Snapshotter` and `ExtensionSnapshotter` interface for modules to implement snapshotters, which will handle both taking 
 snapshot and the restoration. Each module could have mutiple snapshotters, and for modules with additional state, they should
-implement `NamedSnapshotter` as extension snapshotters. When setting up the application, the snapshot `Manager` should call 
-`RegisterExtensions([]NamedSnapshotter…)` to register all the extension snapshotters.
+implement `ExtensionSnapshotter` as extension snapshotters. When setting up the application, the snapshot `Manager` should call 
+`RegisterExtensions([]ExtensionSnapshotter…)` to register all the extension snapshotters.
 
 ```proto
 // SnapshotItem is an item contained in a rootmulti.Store snapshot.
@@ -82,13 +82,13 @@ SnapshotExtensionMeta
 {SnapshotExtensionPayload, ...}
 ```
 
-Add `extensions` field to snapshot `Manager` for extension snapshotters. Multistore snapshotter is a special one and it doesn't need a name because it is always placed at the beginning of the binary stream.
+Add `extensions` field to snapshot `Manager` for extension snapshotters. `multistore` snapshotter is a special one and it doesn't need a name because it is always placed at the beginning of the binary stream.
 
 ```go
 type Manager struct {
 	store      *Store
 	multistore types.Snapshotter
-	extensions map[string]types.NamedSnapshotter
+	extensions map[string]types.ExtensionSnapshotter
     mtx                sync.Mutex
 	operation          operation
 	chRestore          chan<- io.ReadCloser
@@ -98,46 +98,52 @@ type Manager struct {
 }
 ```
 
-For extension snapshotters that implement the `NamedSnapshotter` interface, their names should be registered to the snapshot `Manager` by 
+For extension snapshotters that implement the `ExtensionSnapshotter` interface, their names should be registered to the snapshot `Manager` by 
 calling `RegisterExtensions` when setting up the application. And the snapshotters will handle both taking snapshot and restoration.
 
 ```go
 // RegisterExtensions register extension snapshotters to manager
-func (m *Manager) RegisterExtensions(extensions ...types.NamedSnapshotter) error 
+func (m *Manager) RegisterExtensions(extensions ...types.ExtensionSnapshotter) error 
 ```
 
-As with `Snapshotter` interface for `multistore` snapshotter, two more function signatures: `SnapshotFormat()` and `SupportedFormats()` are added.
+Remain `Snapshotter` interface for `multistore` snapshotter. No need for `SnapshotFormat` and `SupportedFormats` as for `multistore` snapshotter
+formats are handled in a higher level.
 <ul>
 <li> removal of format parameter: `Snapshotter` chooses a format autonomously, not pass in from the caller. </li>
-<li> `Snapshotter` announces the format it uses to snapshot with method `SnapshotFormat()`. </li>
 <ul>
 
 ```go
+// Snapshotter is something that can create and restore snapshots, consisting of streamed binary
+// chunks - all of which must be read from the channel and closed. If an unsupported format is
+// given, it must return ErrUnknownFormat (possibly wrapped with fmt.Errorf).
 type Snapshotter interface {
 	// Snapshot writes snapshot items into the protobuf writer.
-	Snapshot(height uint64, protoWriter protoio.WriteCloser) error
+	Snapshot(height uint64, protoWriter protoio.Writer) error
 
 	// Restore restores a state snapshot from the protobuf items read from the reader.
 	// If the ready channel is non-nil, it returns a ready signal (by being closed) once the
 	// restorer is ready to accept chunks.
-	Restore(height uint64, format uint32, protoReader protoio.ReadCloser) (SnapshotItem, error)
-
-	// SnapshotFormat returns the default format used to take a snapshot.
-	SnapshotFormat() uint32
-
-	// SupportedFormats returns a list of formats it can restore from.
-	SupportedFormats() []uint32
+	Restore(height uint64, format uint32, protoReader protoio.Reader) (SnapshotItem, error)
 }
 ```
 
-Add `NamedSnapshotter` interface for extension snapshotters.
+Add `ExtensionSnapshotter` interface for extension snapshotters, and three more function signatures: `SnapshotFormat()` `SupportedFormats()` and
+`SnapshotName()` are added.
 
 ```go
-type NamedSnapshotter interface {
+// ExtensionSnapshotter is an extension Snapshotter that is appended to the snapshot stream.
+// ExtensionSnapshotter has an unique name and manages it's own internal formats.
+type ExtensionSnapshotter interface {
 	Snapshotter
 
-	// SnapshotterName returns the name of snapshotter, it should be unique in the manager.
-	SnapshotterName() string
+	// SnapshotName returns the name of snapshotter, it should be unique in the manager.
+	SnapshotName() string
+
+	// SnapshotFormat returns the default format used to take a snapshot.
+	SnapshotFormat() uint32
+
+	// SupportedFormats returns a list of formats it can restore from.
+	SupportedFormats() []uint32
 }
 ```
 
@@ -148,7 +154,7 @@ As a result of this implementation, we are able to create snapshots of binary ch
 
 ### Backwards Compatibility
 
-This ADR introduces new proto message types, add field for extension snapshotters to snapshot `Manager`, add new function signatures `SnapshotFormat()`, `SupportedFormats()` to `Snapshotter` interface and add new `NamedSnapshotter` interface, so this is not backwards compatible.
+This ADR introduces new proto message types, add field for extension snapshotters to snapshot `Manager`, add new function signatures `SnapshotFormat()`, `SupportedFormats()` to `Snapshotter` interface and add new `ExtensionSnapshotter` interface, so this is not backwards compatible.
 
 ### Positive
 
@@ -160,7 +166,7 @@ State maintained outside of IAVL tree like CosmWasm blobs can create snapshots b
 
 ### Neutral
 
-All modules that maintain state outside of IAVL tree need to implement `NamedSnapshotter` and the snapshot `Manager` need to call `RegisterExtensions` when setting up the application.
+All modules that maintain state outside of IAVL tree need to implement `ExtensionSnapshotter` and the snapshot `Manager` need to call `RegisterExtensions` when setting up the application.
 
 ## Further Discussions
 

From 6b345620344c8043dd0d3d6f882376a17fa0ec77 Mon Sep 17 00:00:00 2001
From: Adu <foriteration@gmail.com>
Date: Mon, 7 Feb 2022 09:46:36 +0800
Subject: [PATCH 05/12] add details about hash of external data

---
 docs/architecture/adr-049-state-sync-hooks.md | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/docs/architecture/adr-049-state-sync-hooks.md b/docs/architecture/adr-049-state-sync-hooks.md
index 6cce9b659609..5fcd36d9a91d 100644
--- a/docs/architecture/adr-049-state-sync-hooks.md
+++ b/docs/architecture/adr-049-state-sync-hooks.md
@@ -22,8 +22,9 @@ the IAVL tree, they can not add their states to the snapshot stream for state-sy
 
 A simple proposal based on our existing implementation is that, we can add two new message types: `SnapshotExtensionMeta` 
 and `SnapshotExtensionPayload`, and they are appended to the existing multi-store stream with `SnapshotExtensionMeta` 
-acting as a delimiter between extensions. As the chunk hashes should be able to ensure data integrity, we don't need a
-delimiter to mark the end of the snapshot stream.
+acting as a delimiter between extensions. Even for modules that maintain the data outside of the tree, for determinism we
+require that the hash of the external data should be posted in the IAVL tree. As the chunk hashes should be able to ensure 
+data integrity, we don't need a delimiter to mark the end of the snapshot stream.
 
 Besides, we provide `Snapshotter` and `ExtensionSnapshotter` interface for modules to implement snapshotters, which will handle both taking 
 snapshot and the restoration. Each module could have mutiple snapshotters, and for modules with additional state, they should

From 86bee9641630a4f912556dd9a35c5dd59e9b4b62 Mon Sep 17 00:00:00 2001
From: Adu <foriteration@gmail.com>
Date: Mon, 7 Feb 2022 11:36:33 +0800
Subject: [PATCH 06/12] fix misunderstanding of chunk hashes and merkle tree
 hashes

---
 docs/architecture/adr-049-state-sync-hooks.md | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/docs/architecture/adr-049-state-sync-hooks.md b/docs/architecture/adr-049-state-sync-hooks.md
index 5fcd36d9a91d..2deca2fe93c3 100644
--- a/docs/architecture/adr-049-state-sync-hooks.md
+++ b/docs/architecture/adr-049-state-sync-hooks.md
@@ -18,13 +18,15 @@ New clients use state-sync to download snapshots of module state from peer nodes
 stream of `SnapshotStoreItem` and `SnapshotIAVLItem`, which means for app modules that maintain their states outside of
 the IAVL tree, they can not add their states to the snapshot stream for state-sync.
 
+Notification: `Even though the module state data is outside of the tree, for determinism we
+require that the hash of the external data should be posted in the IAVL tree`
+
 ## Decision
 
 A simple proposal based on our existing implementation is that, we can add two new message types: `SnapshotExtensionMeta` 
 and `SnapshotExtensionPayload`, and they are appended to the existing multi-store stream with `SnapshotExtensionMeta` 
-acting as a delimiter between extensions. Even for modules that maintain the data outside of the tree, for determinism we
-require that the hash of the external data should be posted in the IAVL tree. As the chunk hashes should be able to ensure 
-data integrity, we don't need a delimiter to mark the end of the snapshot stream.
+acting as a delimiter between extensions. As the chunk hashes should be able to ensure data integrity, we don't need 
+a delimiter to mark the end of the snapshot stream.
 
 Besides, we provide `Snapshotter` and `ExtensionSnapshotter` interface for modules to implement snapshotters, which will handle both taking 
 snapshot and the restoration. Each module could have mutiple snapshotters, and for modules with additional state, they should

From 7e46ff93f6531c93a85f6693c08911ac01bfec6c Mon Sep 17 00:00:00 2001
From: Adu <foriteration@gmail.com>
Date: Mon, 14 Feb 2022 17:36:51 +0800
Subject: [PATCH 07/12] fix some formating and grammatical errors

---
 docs/architecture/adr-049-state-sync-hooks.md | 69 +++++--------------
 1 file changed, 17 insertions(+), 52 deletions(-)

diff --git a/docs/architecture/adr-049-state-sync-hooks.md b/docs/architecture/adr-049-state-sync-hooks.md
index 2deca2fe93c3..f798e942ff45 100644
--- a/docs/architecture/adr-049-state-sync-hooks.md
+++ b/docs/architecture/adr-049-state-sync-hooks.md
@@ -10,16 +10,17 @@ Draft, Under Implementation
 
 ## Abstract
 
-This ADR provides hooks for app modules to publish snapshot of additional state(outside of IAVL tree) for state-sync.
+This ADR outlines a hooks-based mechanism for application modules to provide additional state (outside of the IAVL tree) to be used 
+during state sync.
 
 ## Context
 
-New clients use state-sync to download snapshots of module state from peer nodes. Currently, the snapshot consists of a
-stream of `SnapshotStoreItem` and `SnapshotIAVLItem`, which means for app modules that maintain their states outside of
-the IAVL tree, they can not add their states to the snapshot stream for state-sync.
+New clients use state-sync to download snapshots of module state from peers. Currently, the snapshot consists of a
+stream of `SnapshotStoreItem` and `SnapshotIAVLItem`, which means that application modules that define their state outside of the IAVL 
+tree cannot include their state as part of the state-sync process.
 
-Notification: `Even though the module state data is outside of the tree, for determinism we
-require that the hash of the external data should be posted in the IAVL tree`
+Note, Even though the module state data is outside of the tree, for determinism we require that the hash of the external data should 
+be posted in the IAVL tree.
 
 ## Decision
 
@@ -35,6 +36,7 @@ implement `ExtensionSnapshotter` as extension snapshotters. When setting up the
 
 ```proto
 // SnapshotItem is an item contained in a rootmulti.Store snapshot.
+// Except the exsiting SnapshotStoreItem and SnapshotIAVLItem, we add two new options for the item.
 message SnapshotItem {
   // item is the specific type of snapshot item.
   oneof item {
@@ -45,24 +47,13 @@ message SnapshotItem {
   }
 }
 
-// SnapshotStoreItem contains metadata about a snapshotted store.
-message SnapshotStoreItem {
-  string name = 1;
-}
-
-// SnapshotIAVLItem is an exported IAVL node.
-message SnapshotIAVLItem {
-  bytes key     = 1;
-  bytes value   = 2;
-  int64 version = 3;
-  int32 height  = 4;
-}
-
 // SnapshotExtensionMeta contains metadata about an external snapshotter.
 // One module may need multiple snapshotters, so each module may have multiple SnapshotExtensionMeta.
 message SnapshotExtensionMeta {
+  // the name of the ExtensionSnapshotter, and it is registered to snapshotter manager when setting up the application
   string name   = 1;
-  // format is used within the snapshotter/namespace, not global one for all modules
+  // this is used by each ExtensionSnapshotter to decide the format of payloads included in SnapshotExtensionPayload message
+  // it is used within the snapshotter/namespace, not global one for all modules
   uint32 format = 2;
 }
 
@@ -72,7 +63,7 @@ message SnapshotExtensionPayload {
 }
 ```
 
-The snapshot stream would look like this:
+The snapshot stream would look like as follows:
 
 ```go
 // multi-store snapshot
@@ -85,7 +76,7 @@ SnapshotExtensionMeta
 {SnapshotExtensionPayload, ...}
 ```
 
-Add `extensions` field to snapshot `Manager` for extension snapshotters. `multistore` snapshotter is a special one and it doesn't need a name because it is always placed at the beginning of the binary stream.
+We add an `extensions` field to snapshot `Manager` for extension snapshotters. The `multistore` snapshotter is a special one and it doesn't need a name because it is always placed at the beginning of the binary stream.
 
 ```go
 type Manager struct {
@@ -102,36 +93,14 @@ type Manager struct {
 ```
 
 For extension snapshotters that implement the `ExtensionSnapshotter` interface, their names should be registered to the snapshot `Manager` by 
-calling `RegisterExtensions` when setting up the application. And the snapshotters will handle both taking snapshot and restoration.
+calling `RegisterExtensions` when setting up the application. The snapshotters will handle both taking snapshot and restoration.
 
 ```go
 // RegisterExtensions register extension snapshotters to manager
 func (m *Manager) RegisterExtensions(extensions ...types.ExtensionSnapshotter) error 
 ```
 
-Remain `Snapshotter` interface for `multistore` snapshotter. No need for `SnapshotFormat` and `SupportedFormats` as for `multistore` snapshotter
-formats are handled in a higher level.
-<ul>
-<li> removal of format parameter: `Snapshotter` chooses a format autonomously, not pass in from the caller. </li>
-<ul>
-
-```go
-// Snapshotter is something that can create and restore snapshots, consisting of streamed binary
-// chunks - all of which must be read from the channel and closed. If an unsupported format is
-// given, it must return ErrUnknownFormat (possibly wrapped with fmt.Errorf).
-type Snapshotter interface {
-	// Snapshot writes snapshot items into the protobuf writer.
-	Snapshot(height uint64, protoWriter protoio.Writer) error
-
-	// Restore restores a state snapshot from the protobuf items read from the reader.
-	// If the ready channel is non-nil, it returns a ready signal (by being closed) once the
-	// restorer is ready to accept chunks.
-	Restore(height uint64, format uint32, protoReader protoio.Reader) (SnapshotItem, error)
-}
-```
-
-Add `ExtensionSnapshotter` interface for extension snapshotters, and three more function signatures: `SnapshotFormat()` `SupportedFormats()` and
-`SnapshotName()` are added.
+Except the existing `Snapshotter` interface for the `multistore`, we add `ExtensionSnapshotter` interface for the extension snapshotters. Three more function signatures: `SnapshotFormat()`, `SupportedFormats()` and `SnapshotName()` are added to `ExtensionSnapshotter`.
 
 ```go
 // ExtensionSnapshotter is an extension Snapshotter that is appended to the snapshot stream.
@@ -161,15 +130,11 @@ This ADR introduces new proto message types, add field for extension snapshotter
 
 ### Positive
 
-State maintained outside of IAVL tree like CosmWasm blobs can create snapshots by implementing extension snapshotters, and being fetched by new clients via state-sync.
-
-### Negative
-
-// Todo
+- State maintained outside of IAVL tree like CosmWasm blobs can create snapshots by implementing extension snapshotters, and being fetched by new clients via state-sync.
 
 ### Neutral
 
-All modules that maintain state outside of IAVL tree need to implement `ExtensionSnapshotter` and the snapshot `Manager` need to call `RegisterExtensions` when setting up the application.
+- All modules that maintain state outside of IAVL tree need to implement `ExtensionSnapshotter` and the snapshot `Manager` need to call `RegisterExtensions` when setting up the application.
 
 ## Further Discussions
 

From bf67fc309add5f52713cb6e1033c39b8fb2f1b39 Mon Sep 17 00:00:00 2001
From: Adu <foriteration@gmail.com>
Date: Mon, 14 Feb 2022 17:43:51 +0800
Subject: [PATCH 08/12] Update docs/architecture/adr-049-state-sync-hooks.md

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>
---
 docs/architecture/adr-049-state-sync-hooks.md | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/architecture/adr-049-state-sync-hooks.md b/docs/architecture/adr-049-state-sync-hooks.md
index f798e942ff45..dd987cc233c2 100644
--- a/docs/architecture/adr-049-state-sync-hooks.md
+++ b/docs/architecture/adr-049-state-sync-hooks.md
@@ -83,7 +83,7 @@ type Manager struct {
 	store      *Store
 	multistore types.Snapshotter
 	extensions map[string]types.ExtensionSnapshotter
-    mtx                sync.Mutex
+	mtx                sync.Mutex
 	operation          operation
 	chRestore          chan<- io.ReadCloser
 	chRestoreDone      <-chan restoreDone

From 799009851e813e3147485c9dc58de46cd77f51c7 Mon Sep 17 00:00:00 2001
From: Adu <foriteration@gmail.com>
Date: Tue, 15 Feb 2022 10:01:42 +0800
Subject: [PATCH 09/12] Update docs/architecture/adr-049-state-sync-hooks.md

Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com>
---
 docs/architecture/adr-049-state-sync-hooks.md | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/architecture/adr-049-state-sync-hooks.md b/docs/architecture/adr-049-state-sync-hooks.md
index dd987cc233c2..68f52f4ef6e8 100644
--- a/docs/architecture/adr-049-state-sync-hooks.md
+++ b/docs/architecture/adr-049-state-sync-hooks.md
@@ -100,7 +100,7 @@ calling `RegisterExtensions` when setting up the application. The snapshotters w
 func (m *Manager) RegisterExtensions(extensions ...types.ExtensionSnapshotter) error 
 ```
 
-Except the existing `Snapshotter` interface for the `multistore`, we add `ExtensionSnapshotter` interface for the extension snapshotters. Three more function signatures: `SnapshotFormat()`, `SupportedFormats()` and `SnapshotName()` are added to `ExtensionSnapshotter`.
+On top of the existing `Snapshotter` interface for the `multistore`, we add `ExtensionSnapshotter` interface for the extension snapshotters. Three more function signatures: `SnapshotFormat()`, `SupportedFormats()` and `SnapshotName()` are added to `ExtensionSnapshotter`.
 
 ```go
 // ExtensionSnapshotter is an extension Snapshotter that is appended to the snapshot stream.

From 50ef80945e992e2fc466d122e73d46c227d7f95b Mon Sep 17 00:00:00 2001
From: Adu <foriteration@gmail.com>
Date: Tue, 15 Feb 2022 10:56:42 +0800
Subject: [PATCH 10/12] clarify that name of ExtensionSnapshotter is unique

---
 docs/architecture/adr-049-state-sync-hooks.md | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/docs/architecture/adr-049-state-sync-hooks.md b/docs/architecture/adr-049-state-sync-hooks.md
index f798e942ff45..1047138fe707 100644
--- a/docs/architecture/adr-049-state-sync-hooks.md
+++ b/docs/architecture/adr-049-state-sync-hooks.md
@@ -51,6 +51,8 @@ message SnapshotItem {
 // One module may need multiple snapshotters, so each module may have multiple SnapshotExtensionMeta.
 message SnapshotExtensionMeta {
   // the name of the ExtensionSnapshotter, and it is registered to snapshotter manager when setting up the application
+  // name should be unique for each ExtensionSnapshotter as we need to alphabetically order their snapshots to get
+  // deterministic snapshot stream.
   string name   = 1;
   // this is used by each ExtensionSnapshotter to decide the format of payloads included in SnapshotExtensionPayload message
   // it is used within the snapshotter/namespace, not global one for all modules
@@ -63,6 +65,8 @@ message SnapshotExtensionPayload {
 }
 ```
 
+When we create a snapshot stream, the `multistore` snapshot is always placed at the beginning of the binary stream, and other extension snapshots are alphabetically ordered by the name of the corresponding `ExtensionSnapshotter`. 
+
 The snapshot stream would look like as follows:
 
 ```go
@@ -132,6 +136,8 @@ This ADR introduces new proto message types, add field for extension snapshotter
 
 - State maintained outside of IAVL tree like CosmWasm blobs can create snapshots by implementing extension snapshotters, and being fetched by new clients via state-sync.
 
+### Negative
+
 ### Neutral
 
 - All modules that maintain state outside of IAVL tree need to implement `ExtensionSnapshotter` and the snapshot `Manager` need to call `RegisterExtensions` when setting up the application.

From bbc3c521c769232af25ba349d7a2a506da36784c Mon Sep 17 00:00:00 2001
From: Adu <foriteration@gmail.com>
Date: Tue, 15 Feb 2022 11:07:00 +0800
Subject: [PATCH 11/12] fix grammar error

---
 docs/architecture/adr-049-state-sync-hooks.md | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/architecture/adr-049-state-sync-hooks.md b/docs/architecture/adr-049-state-sync-hooks.md
index c4b45bfd25b3..c68838d67aac 100644
--- a/docs/architecture/adr-049-state-sync-hooks.md
+++ b/docs/architecture/adr-049-state-sync-hooks.md
@@ -36,7 +36,7 @@ implement `ExtensionSnapshotter` as extension snapshotters. When setting up the
 
 ```proto
 // SnapshotItem is an item contained in a rootmulti.Store snapshot.
-// Except the exsiting SnapshotStoreItem and SnapshotIAVLItem, we add two new options for the item.
+// On top of the exsiting SnapshotStoreItem and SnapshotIAVLItem, we add two new options for the item.
 message SnapshotItem {
   // item is the specific type of snapshot item.
   oneof item {

From d3fe008bb733408db92a1842f268daf531842f56 Mon Sep 17 00:00:00 2001
From: Adu <foriteration@gmail.com>
Date: Tue, 15 Feb 2022 14:37:48 +0800
Subject: [PATCH 12/12] add backwards compatibility explanation

---
 docs/architecture/adr-049-state-sync-hooks.md | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/docs/architecture/adr-049-state-sync-hooks.md b/docs/architecture/adr-049-state-sync-hooks.md
index c68838d67aac..e1616c2265ba 100644
--- a/docs/architecture/adr-049-state-sync-hooks.md
+++ b/docs/architecture/adr-049-state-sync-hooks.md
@@ -130,7 +130,9 @@ As a result of this implementation, we are able to create snapshots of binary ch
 
 ### Backwards Compatibility
 
-This ADR introduces new proto message types, add field for extension snapshotters to snapshot `Manager`, add new function signatures `SnapshotFormat()`, `SupportedFormats()` to `Snapshotter` interface and add new `ExtensionSnapshotter` interface, so this is not backwards compatible.
+This ADR introduces new proto message types, add an `extensions` field in snapshot `Manager`, and add new `ExtensionSnapshotter` interface, so this is not backwards compatible if we have extensions.
+
+But for applications that does not have the state data outside of the IAVL tree for any module, the snapshot stream is backwards-compatible.
 
 ### Positive