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

Make contract addresses predictable #996

Merged
merged 1 commit into from
Sep 9, 2022
Merged

Make contract addresses predictable #996

merged 1 commit into from
Sep 9, 2022

Conversation

alpe
Copy link
Contributor

@alpe alpe commented Sep 9, 2022

Resolves #942

This PR introduces the new predictable contract address model.
It is based on hashing (len(checksum) | checksum | len(sender_address) | sender_address | len(label) | label). Please see the issue linked above for more details and discussion

In order to deal with existing accounts in the wasm address space, the following algorithm is applied.

If an account exists:
    assert sequence == 0 && pubkey == nil
    if account type is in accept list:
        use it
    if account type is in prune list:
       overwrite with base account type and burn balance
    else:
        return error
else:
    crease base account

See keeper.go for the concrete implementation.
We had long internal discussions which account types should go into the accept and prune lists and came up with some defaults that support many use cases and are still safe to use. But as we can not know all use cases and preferences, there are new Options to customize these lists and the balance handling.

CLI example to calculate an new contract address

 wasmd query wasm build-address [code-hash] [creator-address] [label] [flags]

@codecov
Copy link

codecov bot commented Sep 9, 2022

Codecov Report

Merging #996 (ccb2fdd) into main (d9f9f91) will decrease coverage by 0.18%.
The diff coverage is 60.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #996      +/-   ##
==========================================
- Coverage   60.53%   60.34%   -0.19%     
==========================================
  Files          51       52       +1     
  Lines        6319     6430     +111     
==========================================
+ Hits         3825     3880      +55     
- Misses       2224     2263      +39     
- Partials      270      287      +17     
Impacted Files Coverage Δ
app/test_helpers.go 0.77% <0.00%> (-0.06%) ⬇️
x/wasm/client/cli/query.go 0.00% <0.00%> (ø)
x/wasm/types/keys.go 51.11% <ø> (ø)
x/wasm/types/tx.go 44.09% <0.00%> (ø)
x/wasm/module.go 48.93% <50.00%> (-0.52%) ⬇️
x/wasm/client/cli/genesis_msg.go 70.03% <53.33%> (-2.04%) ⬇️
x/wasm/keeper/options.go 76.08% <66.66%> (-4.56%) ⬇️
x/wasm/keeper/keeper.go 87.55% <84.72%> (-1.02%) ⬇️
x/wasm/keeper/contract_keeper.go 92.85% <100.00%> (ø)
x/wasm/keeper/genesis.go 88.57% <100.00%> (-0.77%) ⬇️
... and 8 more

"fmt"
"math"
"path/filepath"
"reflect"

Check notice

Code scanning / CodeQL

Sensitive package import

Certain system packages contain functions which may be a possible source of non-determinism
@@ -2,7 +2,9 @@

import (
"fmt"
"reflect"

Check notice

Code scanning / CodeQL

Sensitive package import

Certain system packages contain functions which may be a possible source of non-determinism
@alpe alpe requested a review from ethanfrey September 9, 2022 11:30
Copy link
Member

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

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

Great stuff!

return nil, nil, sdkerrors.Wrap(sdkerrors.ErrUnauthorized, "can not instantiate")
}

contractAddress := BuildContractAddress(codeInfo.CodeHash, creator, label)
if k.HasContractInfo(ctx, contractAddress) {
Copy link
Member

Choose a reason for hiding this comment

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

Important to have this assertion independent of the account type

// But not all account types of other modules are known or may make sense for contracts, therefore we kept this
// decision logic also very flexible and extendable. We provide new options to overwrite the default settings via WithAcceptedAccountTypesOnContractInstantiation and
// WithPruneAccountTypesOnContractInstantiation as constructor arguments
existingAcct := k.accountKeeper.GetAccount(ctx, contractAddress)
Copy link
Member

Choose a reason for hiding this comment

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

Nice clean implementation, helpful comment 👍

// Cosmos SDK address.Module function.
// Internally a key is built containing (len(checksum) | checksum | len(sender_address) | sender_address | len(label) | label).
// All method parameter values must be valid and not be empty or nil.
func BuildContractAddress(checksum []byte, creator sdk.AccAddress, label string) sdk.AccAddress {
Copy link
Member

Choose a reason for hiding this comment

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

Nice, well defined algorithm.

There was some discussion on including the initMsg data (or a hash of it) as well, to be compatible with CREATE2 from Ethereum. I don't remember the decision regarding this. The other data, and using the label string as user defined nonce is great. Including initMsg may make it harder for some of the use cases (like an exchange pre-generating many receive addresses) that were requested in the original issue.

I am undecided here, and it probably is better without initMsg, just want to make this decision explicit.

contractAccount := k.accountKeeper.NewAccountWithAddress(ctx, contractAddress)
k.accountKeeper.SetAccount(ctx, contractAccount)
// also handle balance to not open cases where these accounts are abused and become liquid
if err := k.coinPruner.PruneBalances(ctx, contractAddress); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

Nice. No tricks by "auto-unvesting" those tokens when we replace the vesting account with base account

// when they exist for an address on contract instantiation.
//
// Values should be references and contain the `*authtypes.BaseAccount` as default bank account type.
func WithAcceptedAccountTypesOnContractInstantiation(accts ...authtypes.AccountI) Option {
Copy link
Member

Choose a reason for hiding this comment

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

Nice helper options

@alpe
Copy link
Contributor Author

alpe commented Sep 9, 2022

Thanks for the feedback. Merging this now to set a RC tag today for further tests. Please provide more feedback. This is not set in stone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make contract addresses predictable ("deterministic")
2 participants