Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

x/cap: fix comments, SetIndex, and some tests #7845

Merged
merged 3 commits into from
Nov 7, 2020
Merged

x/cap: fix comments, SetIndex, and some tests #7845

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5568197
x/cap: fix comments and some tests
ebuchman Nov 6, 2020
87dc12c
fix lint
ebuchman Nov 6, 2020
acbfb5b
Apply suggestions from code review
ebuchman Nov 7, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion x/capability/genesis.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,9 @@ import (
// InitGenesis initializes the capability module's state from a provided genesis
// state.
func InitGenesis(ctx sdk.Context, k keeper.Keeper, genState types.GenesisState) {
k.SetIndex(ctx, genState.Index)
if err := k.SetIndex(ctx, genState.Index); err != nil {
panic(err)
}

// set owners for each index and initialize capability
for _, genOwner := range genState.Owners {
Expand Down
43 changes: 25 additions & 18 deletions x/capability/keeper/keeper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,12 +117,22 @@ func (k *Keeper) InitializeAndSeal(ctx sdk.Context) {
k.sealed = true
}

// SetIndex sets the index to one in InitChain
// Since it is an exported function, we check that index is indeed unset, before initializing
func (k Keeper) SetIndex(ctx sdk.Context, index uint64) {
// SetIndex sets the index to one (or greater) in InitChain according
// to the GenesisState. It must only be called once.
// It will panic if the provided index is 0, or if the index is already set.
func (k Keeper) SetIndex(ctx sdk.Context, index uint64) error {
if index == 0 {
panic("SetIndex requires index > 0")
}
latest := k.GetLatestIndex(ctx)
if latest > 0 {
Comment on lines +127 to +128
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit

Suggested change
latest := k.GetLatestIndex(ctx)
if latest > 0 {
if latest := k.GetLatestIndex(ctx); latest > 0 {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There isn't even a need to store this in a variable.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kind of prefer the clarity of how its written currently for symmetry with the other index value being evaluated.

panic("SetIndex requires index to not be set")
}

// set the global index to the passed index
store := ctx.KVStore(k.storeKey)
store.Set(types.KeyIndex, types.IndexToKey(index))
return nil
}

// GetLatestIndex returns the latest index of the CapabilityKeeper
Expand Down Expand Up @@ -156,7 +166,8 @@ func (k Keeper) GetOwners(ctx sdk.Context, index uint64) (types.CapabilityOwners
}

// InitializeCapability takes in an index and an owners array. It creates the capability in memory
// and sets the fwd and reverse keys for each owner in the memstore
// and sets the fwd and reverse keys for each owner in the memstore.
// It is used during initialization from genesis.
func (k Keeper) InitializeCapability(ctx sdk.Context, index uint64, owners types.CapabilityOwners) {

memStore := ctx.KVStore(k.memKey)
Expand Down Expand Up @@ -278,14 +289,12 @@ func (sk ScopedKeeper) ReleaseCapability(ctx sdk.Context, cap *types.Capability)

memStore := ctx.KVStore(sk.memKey)

// Set the forward mapping between the module and capability tuple and the
// Delete the forward mapping between the module and capability tuple and the
// capability name in the memKVStore
memStore.Delete(types.FwdCapabilityKey(sk.module, cap))

// Set the reverse mapping between the module and capability name and the
// index in the in-memory store. Since marshalling and unmarshalling into a store
// will change memory address of capability, we simply store index as value here
// and retrieve the in-memory pointer to the capability from our map
// Delete the reverse mapping between the module and capability name and the
// index in the in-memory store.
memStore.Delete(types.RevCapabilityKey(sk.module, name))

// remove owner
Expand All @@ -298,7 +307,7 @@ func (sk ScopedKeeper) ReleaseCapability(ctx sdk.Context, cap *types.Capability)
if len(capOwners.Owners) == 0 {
// remove capability owner set
prefixStore.Delete(indexKey)
// since no one ones capability, we can delete capability from map
// since no one owns capability, we can delete capability from map
delete(sk.capMap, cap.GetIndex())
} else {
// update capability owner set
Expand Down Expand Up @@ -370,7 +379,7 @@ func (sk ScopedKeeper) GetOwners(ctx sdk.Context, name string) (*types.Capabilit

// LookupModules returns all the module owners for a given capability
// as a string array and the capability itself.
// The method returns an errors if either the capability or the owners cannot be
// The method returns an error if either the capability or the owners cannot be
// retreived from the memstore.
func (sk ScopedKeeper) LookupModules(ctx sdk.Context, name string) ([]string, *types.Capability, error) {
cap, ok := sk.GetCapability(ctx, name)
Expand Down Expand Up @@ -413,16 +422,14 @@ func (sk ScopedKeeper) getOwners(ctx sdk.Context, cap *types.Capability) *types.

bz := prefixStore.Get(indexKey)

var owners *types.CapabilityOwners
if len(bz) == 0 {
owners = types.NewCapabilityOwners()
} else {
var capOwners types.CapabilityOwners
sk.cdc.MustUnmarshalBinaryBare(bz, &capOwners)
owners = &capOwners
return types.NewCapabilityOwners()
}

return owners
var capOwners types.CapabilityOwners
sk.cdc.MustUnmarshalBinaryBare(bz, &capOwners)
return &capOwners

ebuchman marked this conversation as resolved.
Show resolved Hide resolved
}

func logger(ctx sdk.Context) log.Logger {
Expand Down
28 changes: 22 additions & 6 deletions x/capability/keeper/keeper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,11 +75,15 @@ func (suite *KeeperTestSuite) TestInitializeAndSeal() {
func (suite *KeeperTestSuite) TestNewCapability() {
sk := suite.keeper.ScopeToModule(banktypes.ModuleName)

got, ok := sk.GetCapability(suite.ctx, "transfer")
suite.Require().False(ok)
suite.Require().Nil(got)

cap, err := sk.NewCapability(suite.ctx, "transfer")
suite.Require().NoError(err)
suite.Require().NotNil(cap)

got, ok := sk.GetCapability(suite.ctx, "transfer")
got, ok = sk.GetCapability(suite.ctx, "transfer")
suite.Require().True(ok)
suite.Require().Equal(cap, got)
suite.Require().True(cap == got, "expected memory addresses to be equal")
Expand All @@ -88,9 +92,19 @@ func (suite *KeeperTestSuite) TestNewCapability() {
suite.Require().False(ok)
suite.Require().Nil(got)

cap, err = sk.NewCapability(suite.ctx, "transfer")
got, ok = sk.GetCapability(suite.ctx, "transfer")
suite.Require().True(ok)
suite.Require().Equal(cap, got)
suite.Require().True(cap == got, "expected memory addresses to be equal")

cap2, err := sk.NewCapability(suite.ctx, "transfer")
suite.Require().Error(err)
suite.Require().Nil(cap)
suite.Require().Nil(cap2)

got, ok = sk.GetCapability(suite.ctx, "transfer")
suite.Require().True(ok)
suite.Require().Equal(cap, got)
suite.Require().True(cap == got, "expected memory addresses to be equal")
}

func (suite *KeeperTestSuite) TestOriginalCapabilityKeeper() {
Expand All @@ -111,7 +125,8 @@ func (suite *KeeperTestSuite) TestAuthenticateCapability() {
suite.Require().NoError(err)
suite.Require().NotNil(cap1)

forgedCap := types.NewCapability(0) // index should be the same index as the first capability
forgedCap := types.NewCapability(cap1.Index) // index should be the same index as the first capability
suite.Require().False(sk1.AuthenticateCapability(suite.ctx, forgedCap, "transfer"))
suite.Require().False(sk2.AuthenticateCapability(suite.ctx, forgedCap, "transfer"))

cap2, err := sk2.NewCapability(suite.ctx, "bond")
Expand Down Expand Up @@ -176,14 +191,15 @@ func (suite *KeeperTestSuite) TestGetOwners() {
// Ensure all scoped keepers can get owners
for _, sk := range sks {
owners, ok := sk.GetOwners(suite.ctx, "transfer")
mods, cap, err := sk.LookupModules(suite.ctx, "transfer")
mods, gotCap, err := sk.LookupModules(suite.ctx, "transfer")

suite.Require().True(ok, "could not retrieve owners")
suite.Require().NotNil(owners, "owners is nil")

suite.Require().NoError(err, "could not retrieve modules")
suite.Require().NotNil(cap, "capability is nil")
suite.Require().NotNil(gotCap, "capability is nil")
suite.Require().NotNil(mods, "modules is nil")
suite.Require().Equal(cap, gotCap, "caps not equal")

suite.Require().Equal(len(expectedOrder), len(owners.Owners), "length of owners is unexpected")
for i, o := range owners.Owners {
Expand Down