-
Notifications
You must be signed in to change notification settings - Fork 412
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
Conversation
Codecov Report
@@ 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
|
"fmt" | ||
"math" | ||
"path/filepath" | ||
"reflect" |
Check notice
Code scanning / CodeQL
Sensitive package import
@@ -2,7 +2,9 @@ | |||
|
|||
import ( | |||
"fmt" | |||
"reflect" |
Check notice
Code scanning / CodeQL
Sensitive package import
There was a problem hiding this 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) { |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice helper options
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. |
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 discussionIn order to deal with existing accounts in the wasm address space, the following algorithm is applied.
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