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.

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

feat(x/accounts)!: make address generation more robust and add predictable address creation (backport #22776) #22805

Merged
merged 2 commits into from
Dec 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion tests/integration/accounts/base_account_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ func TestBaseAccount(t *testing.T) {

_, baseAccountAddr, err := ak.Init(ctx, "base", accCreator, &baseaccountv1.MsgInit{
PubKey: toAnyPb(t, privKey.PubKey()),
}, nil)
}, nil, nil)
require.NoError(t, err)

// fund base account! this will also cause an auth base account to be created
Expand Down
2 changes: 1 addition & 1 deletion tests/integration/accounts/fixture_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ func initFixture(t *testing.T, f func(ctx context.Context, msg *account_abstract
banktypes.RegisterMsgServer(router, bankkeeper.NewMsgServerImpl(bankKeeper))

// init account
_, addr, err := accountsKeeper.Init(integrationApp.Context(), "mock", []byte("system"), &gogotypes.Empty{}, nil)
_, addr, err := accountsKeeper.Init(integrationApp.Context(), "mock", []byte("system"), &gogotypes.Empty{}, nil, nil)
require.NoError(t, err)

fixture := &fixture{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ func (s *IntegrationTestSuite) TestContinuousLockingAccount() {
StartTime: currentTime,
// end time in 1 minutes
EndTime: currentTime.Add(time.Minute),
}, sdk.Coins{sdk.NewCoin("stake", math.NewInt(1000))})
}, sdk.Coins{sdk.NewCoin("stake", math.NewInt(1000))}, nil)
require.NoError(t, err)

addr, err := app.AuthKeeper.AddressCodec().BytesToString(randAcc)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ func (s *IntegrationTestSuite) TestDelayedLockingAccount() {
Owner: ownerAddrStr,
// end time in 1 minutes
EndTime: currentTime.Add(time.Minute),
}, sdk.Coins{sdk.NewCoin("stake", math.NewInt(1000))})
}, sdk.Coins{sdk.NewCoin("stake", math.NewInt(1000))}, nil)
require.NoError(t, err)

addr, err := app.AuthKeeper.AddressCodec().BytesToString(randAcc)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ func (s *IntegrationTestSuite) TestPeriodicLockingAccount() {
Length: time.Minute,
},
},
}, sdk.Coins{sdk.NewCoin("stake", math.NewInt(1500))})
}, sdk.Coins{sdk.NewCoin("stake", math.NewInt(1500))}, nil)
require.NoError(t, err)

addr, err := app.AuthKeeper.AddressCodec().BytesToString(randAcc)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ func (s *IntegrationTestSuite) TestPermanentLockingAccount() {

_, accountAddr, err := app.AccountsKeeper.Init(ctx, lockupaccount.PERMANENT_LOCKING_ACCOUNT, accOwner, &types.MsgInitLockupAccount{
Owner: ownerAddrStr,
}, sdk.Coins{sdk.NewCoin("stake", math.NewInt(1000))})
}, sdk.Coins{sdk.NewCoin("stake", math.NewInt(1000))}, nil)
require.NoError(t, err)

addr, err := app.AuthKeeper.AddressCodec().BytesToString(randAcc)
Expand Down
2 changes: 1 addition & 1 deletion tests/integration/accounts/multisig/test_suite.go
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ func (s *IntegrationTestSuite) initAccount(ctx context.Context, sender []byte, m
Revote: false,
EarlyExecution: true,
},
}, sdk.Coins{sdk.NewCoin("stake", math.NewInt(1000))})
}, sdk.Coins{sdk.NewCoin("stake", math.NewInt(1000))}, nil)
s.NoError(err)

accountAddrStr, err := s.app.AuthKeeper.AddressCodec().BytesToString(accountAddr)
Expand Down
2 changes: 1 addition & 1 deletion tests/integration/accounts/wiring_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ func TestDependencies(t *testing.T) {

_, counterAddr, err := ak.Init(ctx, "counter", accCreator, &counterv1.MsgInit{
InitialValue: 0,
}, nil)
}, nil, nil)
require.NoError(t, err)
// test dependencies
creatorInitFunds := sdk.NewCoins(sdk.NewInt64Coin("stake", 100_000))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ func TestAuthToAccountsGRPCCompat(t *testing.T) {

// init three accounts
for n, a := range accs {
_, addr, err := f.accountsKeeper.Init(f.app.Context(), n, []byte("me"), &gogotypes.Empty{}, nil)
_, addr, err := f.accountsKeeper.Init(f.app.Context(), n, []byte("me"), &gogotypes.Empty{}, nil, nil)
require.NoError(t, err)
a.(*mockRetroCompatAccount).address = addr
}
Expand Down Expand Up @@ -132,10 +132,10 @@ func TestAccountsBaseAccountRetroCompat(t *testing.T) {
require.NoError(t, err)

// we init two accounts to have account num not be zero.
_, _, err = f.accountsKeeper.Init(f.app.Context(), "base", []byte("me"), &basev1.MsgInit{PubKey: anyPk}, nil)
_, _, err = f.accountsKeeper.Init(f.app.Context(), "base", []byte("me"), &basev1.MsgInit{PubKey: anyPk}, nil, nil)
require.NoError(t, err)

_, addr, err := f.accountsKeeper.Init(f.app.Context(), "base", []byte("me"), &basev1.MsgInit{PubKey: anyPk}, nil)
_, addr, err := f.accountsKeeper.Init(f.app.Context(), "base", []byte("me"), &basev1.MsgInit{PubKey: anyPk}, nil, nil)
require.NoError(t, err)

// try to query it via auth
Expand Down
26 changes: 26 additions & 0 deletions x/accounts/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -496,6 +496,32 @@ func (a Account) AuthRetroCompatibility(ctx context.Context, _ *authtypes.QueryL
* Implement this handler only for account types you want to expose via x/auth gRPC methods.
* The `info` field in the response can be nil if your account doesn't fit the `BaseAccount` structure.

## Address Derivation

The x/accounts module offers two methods for deriving addresses, both ensuring non-squattability. This means each address is uniquely tied to its creator, preventing address collisions between different creators (e.g., Alice cannot create addresses that would conflict with Bob's addresses).

### Method 1: Using Address Seeds

When creating an account via `MsgInit`, you can provide an `address_seed`. The address is derived using:

```bash
address = sha256(ModuleName || address_seed || creator_address)
```

### Method 2: Using Account Numbers
If no address seed is provided, the address is derived using:

```
address = sha256(ModuleName || creator_address || next_account_number)
```

### Address Seed Best Practices

1. Address seeds must be unique per creator (not globally unique)
2. Reusing an address seed will cause account creation to fail
3. For programmatic account creation, use an incrementing sequence number as the address seed
4. This is particularly useful for contracts or modules that need deterministic address generation

## Genesis

### Creating accounts on genesis
Expand Down
2 changes: 2 additions & 0 deletions x/accounts/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,4 +10,6 @@ var (
ErrBundlerPayment = errors.New(ModuleName, 2, "bundler payment failed")
// ErrExecution is returned when the execution fails.
ErrExecution = errors.New(ModuleName, 3, "execution failed")
// ErrAccountAlreadyExists is returned when the account already exists in state.
ErrAccountAlreadyExists = errors.New(ModuleName, 4, "account already exists")
)
6 changes: 3 additions & 3 deletions x/accounts/genesis_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,13 @@ func TestGenesis(t *testing.T) {
// we init two accounts of the same type

// we set counter to 10
_, addr1, err := k.Init(ctx, testAccountType, []byte("sender"), &types.Empty{}, nil)
_, addr1, err := k.Init(ctx, testAccountType, []byte("sender"), &types.Empty{}, nil, nil)
require.NoError(t, err)
_, err = k.Execute(ctx, addr1, []byte("sender"), &types.UInt64Value{Value: 10}, nil)
require.NoError(t, err)

// we set counter to 20
_, addr2, err := k.Init(ctx, testAccountType, []byte("sender"), &types.Empty{}, nil)
_, addr2, err := k.Init(ctx, testAccountType, []byte("sender"), &types.Empty{}, nil, nil)
require.NoError(t, err)
_, err = k.Execute(ctx, addr2, []byte("sender"), &types.UInt64Value{Value: 20}, nil)
require.NoError(t, err)
Expand Down Expand Up @@ -62,7 +62,7 @@ func TestGenesis(t *testing.T) {
require.Equal(t, &types.UInt64Value{Value: 20}, resp)

// check initted on genesis account
addr3, err := k.makeAddress(2)
addr3, err := k.makeAddress([]byte("sender-2"), 2, nil)
require.NoError(t, err)
resp, err = k.Query(ctx, addr3, &types.DoubleValue{})
require.NoError(t, err)
Expand Down
39 changes: 33 additions & 6 deletions x/accounts/keeper.go
Original file line number Diff line number Diff line change
Expand Up @@ -148,14 +148,15 @@ func (k Keeper) Init(
creator []byte,
initRequest transaction.Msg,
funds sdk.Coins,
addressSeed []byte,
) (transaction.Msg, []byte, error) {
// get the next account number
num, err := k.AccountNumber.Next(ctx)
if err != nil {
return nil, nil, err
}
// create address
accountAddr, err := k.makeAddress(num)
accountAddr, err := k.makeAddress(creator, num, addressSeed)
if err != nil {
return nil, nil, err
}
Expand All @@ -180,7 +181,7 @@ func (k Keeper) initFromMsg(ctx context.Context, initMsg *v1.MsgInit) (transacti
}

// run account creation logic
return k.Init(ctx, initMsg.AccountType, creator, msg, initMsg.Funds)
return k.Init(ctx, initMsg.AccountType, creator, msg, initMsg.Funds, initMsg.AddressSeed)
}

// init initializes the account, given the type, the creator the newly created account number, its address and the
Expand All @@ -199,8 +200,17 @@ func (k Keeper) init(
return nil, fmt.Errorf("%w: not found %s", errAccountTypeNotFound, accountType)
}

// check if account exists
alreadyExists, err := k.AccountsByType.Has(ctx, accountAddr)
if err != nil {
return nil, err
}
if alreadyExists {
return nil, ErrAccountAlreadyExists
}

// send funds, if provided
err := k.maybeSendFunds(ctx, creator, accountAddr, funds)
err = k.maybeSendFunds(ctx, creator, accountAddr, funds)
if err != nil {
return nil, fmt.Errorf("unable to transfer funds: %w", err)
}
Expand Down Expand Up @@ -307,9 +317,26 @@ func (k Keeper) getImplementation(ctx context.Context, addr []byte) (implementat
return impl, nil
}

func (k Keeper) makeAddress(accNum uint64) ([]byte, error) {
// TODO: better address scheme, ref: https://github.com/cosmos/cosmos-sdk/issues/17516
addr := sha256.Sum256(append([]byte("x/accounts"), binary.BigEndian.AppendUint64(nil, accNum)...))
// makeAddress creates an address for the given account.
// It uses the creator address to ensure address squatting cannot happen, for example
// assuming creator sends funds to a new account X nobody can front-run that address instantiation
// unless the creator itself sends the tx.
// AddressSeed can be used to create predictable addresses, security guarantees of the above are retained.
// If address seed is not provided, the address is created using the creator and account number.
func (k Keeper) makeAddress(creator []byte, accNum uint64, addressSeed []byte) ([]byte, error) {
// in case an address seed is provided, we use it to create the address.
var seed []byte
if len(addressSeed) > 0 {
seed = append(creator, addressSeed...)
} else {
// otherwise we use the creator and account number to create the address.
seed = append(creator, binary.BigEndian.AppendUint64(nil, accNum)...)
}

moduleAndSeed := append([]byte(ModuleName), seed...)

addr := sha256.Sum256(moduleAndSeed)

return addr[:], nil
}

Expand Down
8 changes: 4 additions & 4 deletions x/accounts/keeper_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ func TestKeeper_Init(t *testing.T) {
t.Run("ok", func(t *testing.T) {
sender := []byte("sender")

resp, addr, err := m.Init(ctx, "test", sender, &types.Empty{}, nil)
resp, addr, err := m.Init(ctx, "test", sender, &types.Empty{}, nil, nil)
require.NoError(t, err)
require.Equal(t, &types.Empty{}, resp)
require.NotNil(t, addr)
Expand All @@ -34,7 +34,7 @@ func TestKeeper_Init(t *testing.T) {
})

t.Run("unknown account type", func(t *testing.T) {
_, _, err := m.Init(ctx, "unknown", []byte("sender"), &types.Empty{}, nil)
_, _, err := m.Init(ctx, "unknown", []byte("sender"), &types.Empty{}, nil, nil)
require.ErrorIs(t, err, errAccountTypeNotFound)
})
}
Expand All @@ -44,7 +44,7 @@ func TestKeeper_Execute(t *testing.T) {

// create account
sender := []byte("sender")
_, accAddr, err := m.Init(ctx, "test", sender, &types.Empty{}, nil)
_, accAddr, err := m.Init(ctx, "test", sender, &types.Empty{}, nil, nil)
require.NoError(t, err)

t.Run("ok", func(t *testing.T) {
Expand All @@ -70,7 +70,7 @@ func TestKeeper_Query(t *testing.T) {

// create account
sender := []byte("sender")
_, accAddr, err := m.Init(ctx, "test", sender, &types.Empty{}, nil)
_, accAddr, err := m.Init(ctx, "test", sender, &types.Empty{}, nil, nil)
require.NoError(t, err)

t.Run("ok", func(t *testing.T) {
Expand Down
46 changes: 34 additions & 12 deletions x/accounts/msg_server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,24 +24,46 @@ func TestMsgServer(t *testing.T) {
Sender: "sender",
AccountType: "test",
Message: initMsg,
AddressSeed: []byte("seed"),
})
require.NoError(t, err)
require.NotNil(t, initResp)

// execute
executeMsg := &wrapperspb.StringValue{
Value: "10",
}
executeMsgAny, err := implementation.PackAny(executeMsg)
require.NoError(t, err)
t.Run("success", func(t *testing.T) {
// execute
executeMsg := &wrapperspb.StringValue{
Value: "10",
}
executeMsgAny, err := implementation.PackAny(executeMsg)
require.NoError(t, err)

execResp, err := s.Execute(ctx, &v1.MsgExecute{
Sender: "sender",
Target: initResp.AccountAddress,
Message: executeMsgAny,
execResp, err := s.Execute(ctx, &v1.MsgExecute{
Sender: "sender",
Target: initResp.AccountAddress,
Message: executeMsgAny,
})
require.NoError(t, err)
require.NotNil(t, execResp)
})

t.Run("fail initting same account twice", func(t *testing.T) {
_, err := s.Init(ctx, &v1.MsgInit{
Sender: "sender",
AccountType: "test",
Message: initMsg,
AddressSeed: []byte("seed"),
})
require.ErrorIs(t, err, ErrAccountAlreadyExists)
})

t.Run("initting without seed", func(t *testing.T) {
_, err := s.Init(ctx, &v1.MsgInit{
Sender: "sender",
AccountType: "test",
Message: initMsg,
})
require.NoError(t, err)
})
require.NoError(t, err)
require.NotNil(t, execResp)
}

func TestMsgServer_BundlingDisabled(t *testing.T) {
Expand Down
3 changes: 3 additions & 0 deletions x/accounts/proto/cosmos/accounts/v1/tx.proto
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,9 @@ message MsgInit {
// send alongside the request.
repeated cosmos.base.v1beta1.Coin funds = 4
[(gogoproto.castrepeated) = "github.com/cosmos/cosmos-sdk/types.Coins", (gogoproto.nullable) = false];
// address_seed can be used to deterministically create the address of the account.
// If not present the address will be generated based on its associated account number.
bytes address_seed = 5;
}

// MsgInitResponse defines the Create response type for the Msg/Create RPC method.
Expand Down
Loading
Loading