Don't panic during any store operations. (#5294)

* Don't panic during any store operations.

* Panic when creating wrapped store if substores are nil.

* Address Damian's feedback.

(cherry picked from commit b7d6a30)

Author: DimitrisJim

modules/light-clients/08-wasm/types/export_test.go

@@ -32,7 +32,7 @@ func NewMigrateProposalWrappedStore(subjectStore, substituteStore storetypes.KVS
 }
 
 // GetStore is a wrapper around getStore to allow the function to be directly called in tests.
-func (ws migrateClientWrappedStore) GetStore(key []byte) storetypes.KVStore {
+func (ws migrateClientWrappedStore) GetStore(key []byte) (storetypes.KVStore, bool) {
 	return ws.getStore(key)
 }
 

modules/light-clients/08-wasm/types/store.go

@@ -3,7 +3,6 @@ package types
 import (
 	"bytes"
 	"errors"
-	"fmt"
 	"io"
 	"reflect"
 	"strings"
@@ -29,13 +28,20 @@ var (
 //
 // Both stores are used for reads, but only the subjectStore is used for writes. For all operations, the key
 // is checked to determine which store to use and must be prefixed with either "subject/" or "substitute/" accordingly.
-// If the key is not prefixed with either "subject/" or "substitute/", a panic is thrown.
+// If the key is not prefixed with either "subject/" or "substitute/", a default action is taken (e.g. no-op for Set/Delete).
 type migrateClientWrappedStore struct {
 	subjectStore    storetypes.KVStore
 	substituteStore storetypes.KVStore
 }
 
 func newMigrateClientWrappedStore(subjectStore, substituteStore storetypes.KVStore) migrateClientWrappedStore {
+	if subjectStore == nil {
+		panic(errors.New("subjectStore must not be nil"))
+	}
+	if substituteStore == nil {
+		panic(errors.New("substituteStore must not be nil"))
+	}
+
 	return migrateClientWrappedStore{
 		subjectStore:    subjectStore,
 		substituteStore: substituteStore,
@@ -44,11 +50,17 @@ func newMigrateClientWrappedStore(subjectStore, substituteStore storetypes.KVSto
 
 // Get implements the storetypes.KVStore interface. It allows reads from both the subjectStore and substituteStore.
 //
-// Get will panic if the key is not prefixed with either "subject/" or "substitute/".
+// Get will return an empty byte slice if the key is not prefixed with either "subject/" or "substitute/".
 func (ws migrateClientWrappedStore) Get(key []byte) []byte {
 	prefix, key := splitPrefix(key)
 
-	return ws.getStore(prefix).Get(key)
+	store, found := ws.getStore(prefix)
+	if !found {
+		// return a nil byte slice as KVStore.Get() does by default
+		return []byte(nil)
+	}
+
+	return store.Get(key)
 }
 
 // Has implements the storetypes.KVStore interface. It allows reads from both the subjectStore and substituteStore.
@@ -57,59 +69,75 @@ func (ws migrateClientWrappedStore) Get(key []byte) []byte {
 func (ws migrateClientWrappedStore) Has(key []byte) bool {
 	prefix, key := splitPrefix(key)
 
-	return ws.getStore(prefix).Has(key)
+	store, found := ws.getStore(prefix)
+	if !found {
+		// return false as value when store is not found
+		return false
+	}
+
+	return store.Has(key)
 }
 
 // Set implements the storetypes.KVStore interface. It allows writes solely to the subjectStore.
 //
-// Set will panic if the key is not prefixed with "subject/".
+// Set will no-op if the key is not prefixed with "subject/".
 func (ws migrateClientWrappedStore) Set(key, value []byte) {
 	prefix, key := splitPrefix(key)
 	if !bytes.Equal(prefix, subjectPrefix) {
-		panic(fmt.Errorf("writes only allowed on subject store; key must be prefixed with \"%s\"", subjectPrefix))
+		return // no-op
 	}
 
 	ws.subjectStore.Set(key, value)
 }
 
 // Delete implements the storetypes.KVStore interface. It allows deletions solely to the subjectStore.
 //
-// Delete will panic if the key is not prefixed with "subject/".
+// Delete will no-op if the key is not prefixed with "subject/".
 func (ws migrateClientWrappedStore) Delete(key []byte) {
 	prefix, key := splitPrefix(key)
 	if !bytes.Equal(prefix, subjectPrefix) {
-		panic(fmt.Errorf("writes only allowed on subject store; key must be prefixed with \"%s\"", subjectPrefix))
+		return // no-op
 	}
 
 	ws.subjectStore.Delete(key)
 }
 
 // Iterator implements the storetypes.KVStore interface. It allows iteration over both the subjectStore and substituteStore.
 //
-// Iterator will panic if the start or end keys are not prefixed with either "subject/" or "substitute/".
+// Iterator will return a closed iterator if the start or end keys are not prefixed with either "subject/" or "substitute/".
 func (ws migrateClientWrappedStore) Iterator(start, end []byte) storetypes.Iterator {
 	prefixStart, start := splitPrefix(start)
 	prefixEnd, end := splitPrefix(end)
 
 	if !bytes.Equal(prefixStart, prefixEnd) {
-		panic(errors.New("start and end keys must be prefixed with the same prefix"))
+		return ws.closedIterator()
 	}
 
-	return ws.getStore(prefixStart).Iterator(start, end)
+	store, found := ws.getStore(prefixStart)
+	if !found {
+		return ws.closedIterator()
+	}
+
+	return store.Iterator(start, end)
 }
 
 // ReverseIterator implements the storetypes.KVStore interface. It allows iteration over both the subjectStore and substituteStore.
 //
-// ReverseIterator will panic if the start or end keys are not prefixed with either "subject/" or "substitute/".
+// ReverseIterator will return a closed iterator if the start or end keys are not prefixed with either "subject/" or "substitute/".
 func (ws migrateClientWrappedStore) ReverseIterator(start, end []byte) storetypes.Iterator {
 	prefixStart, start := splitPrefix(start)
 	prefixEnd, end := splitPrefix(end)
 
 	if !bytes.Equal(prefixStart, prefixEnd) {
-		panic(errors.New("start and end keys must be prefixed with the same prefix"))
+		return ws.closedIterator()
+	}
+
+	store, found := ws.getStore(prefixStart)
+	if !found {
+		return ws.closedIterator()
 	}
 
-	return ws.getStore(prefixStart).ReverseIterator(start, end)
+	return store.ReverseIterator(start, end)
 }
 
 // GetStoreType implements the storetypes.KVStore interface, it is implemented solely to satisfy the interface.
@@ -127,18 +155,29 @@ func (ws migrateClientWrappedStore) CacheWrapWithTrace(w io.Writer, tc storetype
 	return cachekv.NewStore(tracekv.NewStore(ws, w, tc))
 }
 
-// getStore returns the store to be used for the given key. If the key is prefixed with "subject/", the subjectStore
-// is returned. If the key is prefixed with "substitute/", the substituteStore is returned.
+// getStore returns the store to be used for the given key and a boolean flag indicating if that store was found.
+// If the key is prefixed with "subject/", the subjectStore is returned. If the key is prefixed with "substitute/",
+// the substituteStore is returned.
 //
-// If the key is not prefixed with either "subject/" or "substitute/", a panic is thrown.
-func (ws migrateClientWrappedStore) getStore(prefix []byte) storetypes.KVStore {
+// If the key is not prefixed with either "subject/" or "substitute/", a nil store is returned and the boolean flag is false.
+func (ws migrateClientWrappedStore) getStore(prefix []byte) (storetypes.KVStore, bool) {
 	if bytes.Equal(prefix, subjectPrefix) {
-		return ws.subjectStore
+		return ws.subjectStore, true
 	} else if bytes.Equal(prefix, substitutePrefix) {
-		return ws.substituteStore
+		return ws.substituteStore, true
 	}
 
-	panic(fmt.Errorf("key must be prefixed with either \"%s\" or \"%s\"", subjectPrefix, substitutePrefix))
+	return nil, false
+}
+
+// closedIterator returns an iterator that is always closed, used when Iterator() or ReverseIterator() is called
+// with an invalid prefix or start/end key.
+func (ws migrateClientWrappedStore) closedIterator() storetypes.Iterator {
+	// Create a dummy iterator that is always closed right away.
+	it := ws.subjectStore.Iterator([]byte{0}, []byte{1})
+	it.Close()
+
+	return it
 }
 
 // splitPrefix splits the key into the prefix and the key itself, if the key is prefixed with either "subject/" or "substitute/".