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

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

Merged
merged 4 commits into from
Dec 9, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a9c3b46
add deterministic account creation
testinginprod Dec 5, 2024
5489f66
add docs on addres derivation
testinginprod Dec 5, 2024
cb32a73
go mod tidy all
testinginprod Dec 5, 2024
c81934e
fix conflict
julienrbrt Dec 9, 2024
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
256 changes: 167 additions & 89 deletions api/cosmos/accounts/v1/tx.pulsar.go
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion tests/integration/accounts/base_account_test.go
View file
Open in desktop
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
View file
Open in desktop
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)
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Remove redundant nil parameter.

The Init method call includes two nil parameters, which appears to be redundant. One nil parameter should be sufficient for the address_seed.

-	_, addr, err := accountsKeeper.Init(integrationApp.Context(), "mock", []byte("system"), &gogotypes.Empty{}, nil, nil)
+	_, addr, err := accountsKeeper.Init(integrationApp.Context(), "mock", []byte("system"), &gogotypes.Empty{}, nil)

Committable suggestion skipped: line range outside the PR's diff.

require.NoError(t, err)

fixture := &fixture{
Expand Down
2 changes: 1 addition & 1 deletion tests/integration/accounts/lockup/continous_lockup_test_suite.go
View file
Open in desktop
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
2 changes: 1 addition & 1 deletion tests/integration/accounts/lockup/delayed_lockup_test_suite.go
View file
Open in desktop
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
2 changes: 1 addition & 1 deletion tests/integration/accounts/lockup/periodic_lockup_test_suite.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,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
2 changes: 1 addition & 1 deletion tests/integration/accounts/lockup/permanent_lockup_test_suite.go
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
6 changes: 3 additions & 3 deletions tests/integration/auth/keeper/accounts_retro_compatibility_test.go
View file
Open in desktop
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
6 changes: 3 additions & 3 deletions tests/integration/v2/auth/accounts_retro_compatibility_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ func TestAuthToAccountsGRPCCompat(t *testing.T) {

// init three accounts
for n, a := range accs {
_, addr, err := f.accountsKeeper.Init(f.ctx, n, []byte("me"), &gogotypes.Empty{}, nil)
_, addr, err := f.accountsKeeper.Init(f.ctx, n, []byte("me"), &gogotypes.Empty{}, nil, nil)
require.NoError(t, err)
a.(*mockRetroCompatAccount).address = addr
}
Expand Down Expand Up @@ -159,10 +159,10 @@ func TestAccountsBaseAccountRetroCompat(t *testing.T) {
// we init two accounts. Account number should start with 4
// since the first three accounts are fee_collector, bonded_tokens_pool, not_bonded_tokens_pool
// generated by init genesis plus one more genesis account, which make the current account number 4.
_, _, err = f.accountsKeeper.Init(f.ctx, "base", []byte("me"), &basev1.MsgInit{PubKey: anyPk}, nil)
_, _, err = f.accountsKeeper.Init(f.ctx, "base", []byte("me"), &basev1.MsgInit{PubKey: anyPk}, nil, nil)
require.NoError(t, err)

_, addr, err := f.accountsKeeper.Init(f.ctx, "base", []byte("me"), &basev1.MsgInit{PubKey: anyPk}, nil)
_, addr, err := f.accountsKeeper.Init(f.ctx, "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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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)
Comment on lines +22 to +28
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add test cases for address seed persistence

The genesis test should verify that address seeds are correctly persisted and restored during export/import cycles.

Consider adding test cases that:

  1. Initialize accounts with specific address seeds
  2. Export genesis
  3. Import genesis
  4. Verify address reconstruction

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)
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Add test cases for address generation with non-nil seeds

Based on the implementation in keeper.go, the makeAddress function supports two modes of address generation:

  1. Using creator and account number (current test coverage)
  2. Using creator and addressSeed (untested)

Add test cases to verify the deterministic address creation with non-nil seeds to ensure both code paths work as expected.

Example locations that need attention:

  • x/accounts/genesis_test.go: Add test cases with non-nil addressSeed parameter
🔗 Analysis chain

Verify address generation with non-nil seed.

The test only covers address generation with nil seed. Consider adding test cases with non-nil seeds to verify deterministic address creation.

Let me check the implementation of makeAddress to better understand the address generation logic.

Let me try to find the implementation using ripgrep instead.

Let me check all test files to see if there are any tests using non-nil addressSeed.

Let me try one more search with a different approach.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if there are any tests covering non-nil address seeds
rg -l "makeAddress.*nil" | while read -r file; do
    echo "=== $file ==="
    rg "makeAddress.*[^n][^i][^l]" "$file" || echo "No non-nil seed tests found"
done

Length of output: 252

Script:

#!/bin/bash
# Find the implementation of makeAddress function
ast-grep --pattern 'func $_ makeAddress($$$) {
  $$$
}'

Length of output: 57

Script:

#!/bin/bash
# Find the implementation of makeAddress function
rg -A 10 "func.*makeAddress"

Length of output: 733

Script:

#!/bin/bash
# Search for makeAddress calls in test files
fd -e test.go --exec rg -B2 -A2 "makeAddress.*\(" {} \;

Length of output: 60

Script:

#!/bin/bash
# Search for all makeAddress calls in the codebase
rg -B2 -A2 "makeAddress\("

Length of output: 1070

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
View file
Open in desktop
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)
Comment on lines +151 to 154
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Update function comment to include the new addressSeed parameter

The comment for the Init function should be updated to describe the purpose and usage of the addressSeed parameter, following Go documentation guidelines for exported functions.

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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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