R4R: Store Accessor Types #4514

mossid · 2019-06-07T21:51:15Z

closes: #4554

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] [stanza] [message]
rereviewed 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)

store/mapping/test_common.go

codecov · 2019-06-11T17:24:15Z

Codecov Report

Merging #4514 into master will increase coverage by 0.02%.
The diff coverage is 59.93%.

@@            Coverage Diff            @@
##           master   #4514      +/-   ##
=========================================
+ Coverage   54.98%     55%   +0.02%     
=========================================
  Files         297     305       +8     
  Lines       18225   18514     +289     
=========================================
+ Hits        10021   10184     +163     
- Misses       7466    7581     +115     
- Partials      738     749      +11

alexanderbez · 2019-06-12T15:38:58Z

@mossid before starting a review, can you quickly open an issue outline the rationale and ideas behind this PR and why it's warranted please? Also, address the merge conflicts (but this isn't necessary pre-review) 👍

…-mapping

store/state/base.go

store/state/enum.go

…-mapping

store/state/test_common.go

store/state/mapping.go

…-mapping

fedekunze · 2019-07-18T13:11:35Z

@mossid what's the status of this PR? do you need someone to help out with what's left?

alexanderbez

Thanks @mossid -- performed an initial review. One thing to note is I'd like to see unit tests testing all the behavior of both the generic and types accessors. In addition, I'm not sure if store/state is the best structure...maybe store/accessor?

store/state/base.go

store/state/boolean.go

store/state/enum.go

store/state/indexer.go

store/state/integer.go

store/state/value.go

alexanderbez · 2019-07-22T12:49:55Z

store/state/value.go

+	return v.base.store(ctx)
+}
+
+func (v Value) Cdc() *codec.Codec {


Why do we need a getter for the codec? What if we passed in the codec instead of having it as part of Value?

Updates on this suggestion @mossid ?

Having codec as an argument will make the code bit harder to read but okay with it, will change

alexanderbez · 2019-07-22T12:50:46Z

store/state/value.go

+	return v.base.key(v.key)
+}
+
+type GetSafeErrorType byte


lets move this to a separate errors.go file

alexanderbez · 2019-07-22T12:51:16Z

store/state/mapping.go

+func NewMapping(base Base, prefix []byte) Mapping {
+	return Mapping{
+		base:  base.Prefix(prefix),
+		start: []byte{}, // preventing nil key access in store.Last


This comment really has no context.

store/state/mapping.go

…o joon/store-mapping

…-mapping

alexanderbez

@mossid there seems to be some remaining feedback to be addressed. Also, what are your thoughts in the package being store/accessor instead of store/state? The latter is a bit ambiguous.

alexanderbez · 2019-09-03T13:28:02Z

store/state/mapping.go

+	m.Value(key).Delete(ctx)
+}
+
+func (m Mapping) Cdc() *codec.Codec {


Bump @mossid

alexanderbez · 2019-09-03T13:28:22Z

store/state/types.go

+
+type KVStore = sdk.KVStore
+type Context = sdk.Context
+type CLIContext = context.CLIContext


bump @mossid

Co-Authored-By: Federico Kunze <[email protected]>

…o joon/store-mapping

…-mapping

fedekunze · 2019-09-29T05:58:05Z

store/state/boolean.go

+	Value
+}
+
+func (v Value) Boolean() Boolean {


All these functions should be moved to value.go

func (v Value) Type() Type {

fedekunze · 2019-09-29T05:59:31Z

store/state/enum.go

+	return
+}
+
+func (v Enum) Transit(ctx Context, from, to byte) bool {


I don't understand the name either

fedekunze · 2019-09-29T06:00:20Z

store/state/errors.go

+}
+
+// GetSafeError is error type for GetSafe method
+type GetSafeError struct {


Suggested change

type GetSafeError struct {

type SafeError struct {

I really don't understand why we need an error wrapper type like this one. What happens if GetSafe just returns an error? Why do we want to wrap it? All those answers should be specified on a comment here

fedekunze · 2019-09-29T06:01:43Z

store/state/errors.go

+)
+
+// GetSafeErrorType is enum for indicating the type of error
+type GetSafeErrorType byte


Suggested change

type GetSafeErrorType byte

type SafeErrorType byte

fedekunze · 2019-09-29T06:05:06Z

store/state/errors.go

+// GetSafeError is error type for GetSafe method
+type GetSafeError struct {
+	ty    GetSafeErrorType
+	inner error


Suggested change

inner error

err error

store/state/integer.go

store/state/mapping.go

store/state/value.go

fedekunze · 2019-09-29T06:16:44Z

store/state/value_test.go

+var testkey = sdk.NewKVStoreKey("test")
+
+func init() {
+	// register


fedekunze · 2019-10-01T10:56:44Z

@alexanderbez I noticed there are a bunch of changes to the store accessors on the upstream PRs. I think the best would be to fork #5057 and remove everything that's not related to the accessors.

* Store accessor upstream changes (#5119)

golangcibot · 2019-10-01T11:15:50Z

store/state/types.go

+	}
+
+	return q.store.Query(req), nil
+


unnecessary trailing newline (from whitespace)

golangcibot · 2019-10-01T11:15:50Z

client/context/query.go

-// proof if TrustNode is disabled. If proof verification fails or the query
-// height is invalid, an error will be returned.
-func (ctx CLIContext) query(path string, key cmn.HexBytes) (res []byte, height int64, err error) {
+func (ctx CLIContext) queryABCI(req abci.RequestQuery) (resp abci.ResponseQuery, err error) {


unnecessary leading newline (from whitespace)

golangcibot · 2019-10-01T11:15:51Z

store/state/value_test.go

+	return reflect.ValueOf(ptr).Elem().Interface()
+}
+
+func TestTypeValue(t *testing.T) {


Function 'TestTypeValue' is too long (66 > 60) (from funlen)

golangcibot · 2019-10-01T11:18:16Z

client/context/query.go

 	}

-	err = ctx.verifyProof(path, resp)
+	return


naked return in func queryABCI with 44 lines of code (from nakedret)

Co-Authored-By: Federico Kunze <[email protected]>

golangcibot · 2019-10-01T11:38:36Z

store/state/value_test.go

+}
+
+func (v integerT) Unmarshal(bz []byte, ptr interface{}) {
+	res, err := DecodeInt(bz, v.enc)


v.enc undefined (type integerT has no field or method enc) (from typecheck)

golangcibot · 2019-10-01T11:38:36Z

store/state/value_test.go

+}
+
+func (v valueT) Unmarshal(bz []byte, ptr interface{}) {
+	v.m.cdc.MustUnmarshalBinaryBare(bz, ptr)


v.m undefined (type valueT has no field or method m) (from typecheck)

golangcibot · 2019-10-01T11:38:37Z

store/state/value.go

+}
+
+func (v Value) QueryRaw(q ABCIQuerier) ([]byte, *Proof, error) {
+	resp, err := q.Query(v.m.StoreName(), v.KeyBytes())


v.m undefined (type Value has no field or method m) (from typecheck)

golangcibot · 2019-10-01T11:38:37Z

store/state/value.go

+// NewValue constructs a Value.
+func NewValue(m Mapping, key []byte) Value {
+	return Value{
+		m:   m,


unknown field m in struct literal (from typecheck)

golangcibot · 2019-10-01T11:38:37Z

store/state/integer.go

+		res = 0
+		return
+	}
+	res, err = DecodeInt(value, v.enc)


v.enc undefined (type Integer has no field or method enc) (from typecheck)

golangcibot · 2019-10-01T11:38:38Z

store/state/indexer.go

+// Prefix returns a new Indexer with the updated prefix
+func (ix Indexer) Prefix(prefix []byte) Indexer {
+	return Indexer{
+		m: ix.m.Prefix(prefix),


ix.m undefined (type Indexer has no field or method m) (from typecheck)

golangcibot · 2019-10-01T11:38:38Z

store/state/indexer.go

+	return Indexer{
+		m: ix.m.Prefix(prefix),
+
+		enc: ix.enc,


ix.enc undefined (type Indexer has no field or method enc) (from typecheck)

fedekunze

I have a lot of questions regarding the design decisions of this PR

fedekunze · 2019-10-04T13:44:00Z

store/state/mapping.go

+// Mapping is key []byte -> value []byte mapping using a base(possibly prefixed).
+// All store accessing operations are redirected to the Value corresponding to
+// the key argument.
+type Mapping struct {


Note: these are the basic fields used on a Keeper

fedekunze · 2019-10-04T13:45:01Z

store/state/mapping.go

+	return Mapping{
+		storeKey: m.storeKey,
+		cdc:      m.cdc,
+		prefix:   join(m.prefix, prefix),


the name is misleading, it should just replace the mapping's prefix to another one

fedekunze · 2019-10-04T13:46:24Z

store/state/types.go

+	req := abci.RequestQuery{
+		Path:  "/store/" + storeName + "/key",
+		Data:  key,
+		Prove: true,


why is this hardcoded to true?

fedekunze · 2019-10-04T13:50:27Z

store/state/types.go

+
+var _ ABCIQuerier = CLIQuerier{}
+
+type CLIQuerier struct {


we shouldn't declare all these types. It is really confusing as to what is the difference between the current sdk.Querier and these ones. The new CLI flow is extremely confusing and intruduces a lot of overhead because of the new accessor types. This will significantly diminish the current dev UX on creating modules.

What are the significant benefits of using/declaring these types? There's no documentation on the process of integrating the accessors.

We should integrate the new accessor types to the current system with Queriers or directly calling them from the cliContext

fedekunze · 2019-10-04T13:50:53Z

store/state/types.go

+
+func (q CLIQuerier) Query(storeName string, key []byte) (abci.ResponseQuery, error) {
+	req := abci.RequestQuery{
+		Path:  "/store/" + storeName + "/key",


these prefixes and suffixes are not intuitive at all

fedekunze · 2019-10-04T13:52:09Z

store/state/types.go

+	req := abci.RequestQuery{
+		Path:  "/" + storeName + "/key",
+		Data:  key,
+		Prove: true,


also hardcoded, why?

fedekunze · 2019-10-04T13:59:56Z

store/state/value.go

+	return v.m.KeyBytes(v.key)
+}
+
+func (v Value) QueryRaw(q ABCIQuerier) ([]byte, *Proof, error) {


here you are passing a Querier as a parameter to a function from Value when ideally it should be the other way. This makes things really confusing and forces you to define all the query logic on the client/cli/query .go itself because you need to define cliCtx there.

fedekunze · 2019-10-04T14:00:34Z

store/state/value.go

+	return resp.Value, resp.Proof, nil
+}
+
+func (v Value) Query(q ABCIQuerier, ptr interface{}) (*Proof, error) {


fedekunze · 2019-10-04T14:04:14Z

store/state/value.go

+// Value is a capability for reading and writing on a specific key-value point
+// in the state. Value consists of a mapping and a key. An actor holding a Value
+// has a full access right on that state point.
+type Value struct {


so the Value is part of a KVStore.... but the Value itself has a map "key []byte -> value []byte" and a prefix? This is all super confusing...

Also, why does the value contain the one of the keys of the map?

fedekunze · 2019-10-04T14:13:01Z

store/state/value.go

+
+// KeyBytes returns the prefixed key that the Value is providing to the KVStore.
+func (v Value) KeyBytes() []byte {
+	return v.m.KeyBytes(v.key)


this is the only instance that the Value.key is used. Why do you need to declare it on the type and not pass it as an argument?

This is telling me that Value itself is really a KVPair with the capability to retrieve the actual value from the defined Value.Mapping, it's not really a value, making the name of the type misleadding.

jackzampolin · 2019-10-14T22:10:10Z

It sounds like this feature has been shelved after further review. I vote that we close this implementation PR.

add mapping

9375649

mossid added WIP C:Store labels Jun 7, 2019

golangcibot reviewed Jun 7, 2019

View reviewed changes

store/mapping/test_common.go Outdated Show resolved Hide resolved

rm unused mapping/*, rm interfaces

16eea68

mossid added 2 commits June 12, 2019 00:14

rm unused code

f73215e

mv mapping -> state, rm x/ibc

cf0b8f2

mossid changed the title ~~WIP: Store Accessor Types~~ R4R: Store Accessor Types Jun 11, 2019

mossid added ready-for-review and removed WIP labels Jun 11, 2019

mossid marked this pull request as ready for review June 11, 2019 22:36

mossid requested review from alessio, alexanderbez, jackzampolin and rigelrozanski as code owners June 11, 2019 22:36

Merge branch 'master' of github.com:cosmos/cosmos-sdk into joon/store…

e25f652

…-mapping

fedekunze reviewed Jun 21, 2019

View reviewed changes

store/state/base.go Outdated Show resolved Hide resolved

mossid added 3 commits June 25, 2019 16:11

rm GetIfExists

2b17a39

add key

dc84e1e

rm custom encoding/decoding in enum/bool

19d7155

golangcibot reviewed Jun 29, 2019

View reviewed changes

store/state/enum.go Outdated Show resolved Hide resolved

mossid added 2 commits June 29, 2019 15:07

Merge branch 'master' of github.com:cosmos/cosmos-sdk into joon/store…

6764bc4

…-mapping

fix lint

8affaa7

golangcibot reviewed Jun 29, 2019

View reviewed changes

mossid added 2 commits July 7, 2019 18:23

rm tests

a7898b4

Merge branch 'master' of github.com:cosmos/cosmos-sdk into joon/store…

3939bd4

…-mapping

alexanderbez suggested changes Jul 22, 2019

View reviewed changes

mossid added 2 commits August 15, 2019 20:25

Merge branch 'joon/store-mapping' of github.com:cosmos/cosmos-sdk int…

7471bdf

…o joon/store-mapping

Merge branch 'master' of github.com:cosmos/cosmos-sdk into joon/store…

04543f8

…-mapping

alexanderbez suggested changes Sep 3, 2019

View reviewed changes

mossid force-pushed the joon/store-mapping branch from f52abaf to 04543f8 Compare September 10, 2019 20:48

mossid and others added 6 commits September 10, 2019 23:06

Apply suggestions from code review

10d1c46

Co-Authored-By: Federico Kunze <[email protected]>

applying review in progress

09e6600

Merge branch 'joon/store-mapping' of github.com:cosmos/cosmos-sdk int…

6f5f633

…o joon/store-mapping

apply review - make querier interface

4116bb7

Merge branch 'master' of github.com:cosmos/cosmos-sdk into joon/store…

8040c81

…-mapping

revise querier interface to work both on cli & store

828badd

fedekunze suggested changes Sep 29, 2019

View reviewed changes

fedekunze mentioned this pull request Sep 29, 2019

[ADR-012] - State Accessors #4998

Merged

5 tasks

fedekunze assigned alexanderbez Oct 1, 2019

fedekunze added 2 commits October 1, 2019 13:12

Store accessor upstream changes (#5119)

21bd8d0

* Store accessor upstream changes (#5119)

Merge branch 'master' into joon/store-mapping

9b3b120

golangcibot reviewed Oct 1, 2019

View reviewed changes

Apply suggestions from code review

abd9fec

Co-Authored-By: Federico Kunze <[email protected]>

golangcibot reviewed Oct 1, 2019

View reviewed changes

Merge branch 'master' into joon/store-mapping

a6dadef

fedekunze suggested changes Oct 4, 2019

View reviewed changes

jackzampolin closed this Oct 14, 2019

fedekunze deleted the joon/store-mapping branch December 24, 2019 16:28

R4R: Store Accessor Types #4514

R4R: Store Accessor Types #4514

Conversation

mossid commented Jun 7, 2019 • edited by alexanderbez Loading

codecov bot commented Jun 11, 2019 • edited Loading

Codecov Report

alexanderbez commented Jun 12, 2019

fedekunze commented Jul 18, 2019

alexanderbez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mossid Sep 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanderbez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fedekunze commented Oct 1, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fedekunze left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackzampolin commented Oct 14, 2019

mossid commented Jun 7, 2019 •

edited by alexanderbez

Loading

codecov bot commented Jun 11, 2019 •

edited

Loading

mossid Sep 11, 2019 •

edited

Loading