refactor!: use KVStoreService in x/auth #15520

facundomedica · 2023-03-22T20:55:33Z

Description

Most changes come from switching from sdk.Context to context.Context (changed the keeper methods and we have expected keeper interfaces everywhere for auth).
Right now we still pass the StoreKey to the Keeper instead of the service because we need it for pagination and prefix iteration (prefix.NewStore and query.Paginate). Should we do something about that now?
Added an adapter so we can pass the KVStoreService's KVStore into query.Paginate and prefix.NewStore (not sure about where to put it)

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)

…/auth-storesvc

x/auth/keeper/account.go

+	store := ak.storeSvc.OpenKVStore(ctx)
+	has, err := store.Has(types.AddressStoreKey(addr))
+	if err != nil {
+		panic(err)


x/auth/keeper/account.go

+	store := ak.storeSvc.OpenKVStore(ctx)
+	bz, err := store.Get(types.AddressStoreKey(addr))
+	if err != nil {
+		panic(err)


x/auth/keeper/keeper.go

…/auth-storesvc

…-sdk into facu/auth-storesvc

facundomedica · 2023-03-24T15:19:41Z

runtime/store.go

+func KVStoreAdapter(store store.KVStore) storetypes.KVStore {
+	return &kvStoreAdapter{store}
+}


Not sure where to put this adapter, accepting suggestions :D

why is this needed? the new interface returns errors can we use that or?

Because there's the prefix store and the query.Paginate that take in the KVStore from /store/types instead of core. Once we migrate all modules we could remove this adapter and modify the other methods to take in the core KVStore.

So yes, because the new interface returns errors so it's not compatible

makes sense thank you

facundomedica · 2023-03-24T15:33:49Z

x/feegrant/go.mod

@@ -158,3 +158,4 @@ require (
 // Fix upstream GHSA-h395-qcrw-5vmq vulnerability.
 // TODO Remove it: https://github.com/cosmos/cosmos-sdk/issues/10409
 replace github.com/gin-gonic/gin => github.com/gin-gonic/gin v1.8.1
+replace github.com/cosmos/cosmos-sdk => ../../


We can remove this once we tag

We won't tag the SDK in a while, best to use a pseudo version of the SDK from this branch.

Yeah, I realized later that we are not tagging x/auth separately

x/nft/go.mod

runtime/store.go

+func (s kvStoreAdapter) Set(key []byte, value []byte) {
+	err := s.store.Set(key, value)
+	if err != nil {
+		panic(err)


runtime/store.go

+func (s kvStoreAdapter) Iterator(start []byte, end []byte) dbm.Iterator {
+	it, err := s.store.Iterator(start, end)
+	if err != nil {
+		panic(err)


runtime/store.go

+func (s kvStoreAdapter) Delete(key []byte) {
+	err := s.store.Delete(key)
+	if err != nil {
+		panic(err)


runtime/store.go

+func (s kvStoreAdapter) Get(key []byte) []byte {
+	bz, err := s.store.Get(key)
+	if err != nil {
+		panic(err)


runtime/store.go

+func (s kvStoreAdapter) Has(key []byte) bool {
+	has, err := s.store.Has(key)
+	if err != nil {
+		panic(err)


julienrbrt · 2023-03-24T17:41:12Z

simapp/app.go

@@ -289,7 +289,7 @@ func NewSimApp(
 	bApp.SetParamStore(&app.ConsensusParamsKeeper)

 	// add keepers
-	app.AccountKeeper = authkeeper.NewAccountKeeper(appCodec, keys[authtypes.StoreKey], authtypes.ProtoBaseAccount, maccPerms, sdk.Bech32MainPrefix, authtypes.NewModuleAddress(govtypes.ModuleName).String())
+	app.AccountKeeper = authkeeper.NewAccountKeeper(appCodec, runtime.NewKVStoreService(keys[authtypes.StoreKey]), authtypes.ProtoBaseAccount, maccPerms, sdk.Bech32MainPrefix, authtypes.NewModuleAddress(govtypes.ModuleName).String())


Same comment as before about UPGRADING.md + changelog.
I think now the category can be generalized under simapp. Moreover, we should add that if chains are generating mocks for their own testing, they will need to re-generate them.

Github is taking a while to update stuff here, but I've added a changelog entry and modified the upgrading doc.

julienrbrt

lgtm! left 2 comments.

UPGRADING.md

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>

tac0turtle · 2023-03-27T18:44:32Z

runtime/store.go

+func KVStoreAdapter(store store.KVStore) storetypes.KVStore {
+	return &kvStoreAdapter{store}
+}


makes sense thank you

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>

facundomedica added 3 commits March 22, 2023 17:31

some progress

65e40ff

Merge branch 'main' of https://github.com/cosmos/cosmos-sdk into facu…

c3f100b

…/auth-storesvc

progress

bbc4059

github-actions bot added the C:x/auth label Mar 22, 2023

fixing tests

b7fa375

github-advanced-security bot found potential problems Mar 22, 2023

View reviewed changes

facundomedica added 2 commits March 23, 2023 16:08

Merge branch 'main' of https://github.com/cosmos/cosmos-sdk into facu…

e72edb1

…/auth-storesvc

sdk.Context -> context.Context

a8876c6

github-actions bot added C:Simulations C:x/authz C:x/bank C:x/distribution distribution module related C:x/evidence C:x/feegrant C:x/genutil genutil module issues C:x/gov C:x/group C:x/mint C:x/nft C:x/slashing C:x/staking labels Mar 23, 2023

facundomedica and others added 6 commits March 24, 2023 11:45

add prefix iterator and pagination

7967507

Merge branch 'main' into facu/auth-storesvc

ea197a1

fix legacy app.go

ba37db7

Merge branch 'facu/auth-storesvc' of https://github.com/cosmos/cosmos…

7736215

…-sdk into facu/auth-storesvc

replace for a single adapter

d23760e

replace for a single adapter

3bb304d

facundomedica commented Mar 24, 2023

View reviewed changes

facundomedica added 2 commits March 24, 2023 12:25

fix test

2aeb2c0

fix tests

9bc07f8

facundomedica commented Mar 24, 2023

View reviewed changes

x/nft/go.mod Outdated Show resolved Hide resolved

fix

479238b

facundomedica marked this pull request as ready for review March 24, 2023 15:39

facundomedica requested a review from a team as a code owner March 24, 2023 15:39

fix tests

e7ffab5

github-advanced-security bot found potential problems Mar 24, 2023

View reviewed changes

julienrbrt mentioned this pull request Mar 24, 2023

refactor(feegrant): remove bech32 global #15347

Merged

19 tasks

julienrbrt reviewed Mar 24, 2023

View reviewed changes

julienrbrt approved these changes Mar 24, 2023

View reviewed changes

facundomedica added 5 commits March 27, 2023 09:51

fix conflicts

802aa1b

suggestions

9a19133

upgrading + cl++

9689f99

fix sdk version for nft and feegrant

2bbbd41

fix tests

759e21b

alexanderbez reviewed Mar 27, 2023

View reviewed changes

UPGRADING.md Outdated Show resolved Hide resolved

facundomedica and others added 2 commits March 27, 2023 15:03

Update UPGRADING.md

2abacf7

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>

Merge branch 'main' into facu/auth-storesvc

40b76e0

tac0turtle approved these changes Mar 27, 2023

View reviewed changes

facundomedica enabled auto-merge (squash) March 27, 2023 18:47

facundomedica merged commit d29e8eb into main Mar 27, 2023

facundomedica deleted the facu/auth-storesvc branch March 27, 2023 18:57

faddat mentioned this pull request Mar 28, 2023

update linter to 1521 ChihuahuaChain/cosmos-sdk#6

Closed

19 tasks

larry0x pushed a commit to larry0x/cosmos-sdk that referenced this pull request May 22, 2023

refactor!: use KVStoreService in x/auth (cosmos#15520)

bc1ef17

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor!: use KVStoreService in x/auth #15520

refactor!: use KVStoreService in x/auth #15520

facundomedica commented Mar 22, 2023 •

edited

Loading

facundomedica Mar 24, 2023

tac0turtle Mar 24, 2023

facundomedica Mar 24, 2023

facundomedica Mar 24, 2023

tac0turtle Mar 27, 2023

facundomedica Mar 24, 2023

julienrbrt Mar 24, 2023 •

edited

Loading

facundomedica Mar 24, 2023

julienrbrt Mar 24, 2023 •

edited

Loading

facundomedica Mar 27, 2023

julienrbrt left a comment

tac0turtle Mar 27, 2023

refactor!: use KVStoreService in x/auth #15520

refactor!: use KVStoreService in x/auth #15520

Conversation

facundomedica commented Mar 22, 2023 • edited Loading

Description

Author Checklist

Reviewers Checklist

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

julienrbrt Mar 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julienrbrt Mar 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julienrbrt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facundomedica commented Mar 22, 2023 •

edited

Loading

julienrbrt Mar 24, 2023 •

edited

Loading

julienrbrt Mar 24, 2023 •

edited

Loading