Stop rejecting contract instantiation if account holds funds #738
Conversation
wojtek-coreum
requested review from
dzmitryhil,
miladz68 and
ysv
and removed request for
a team
There was a problem hiding this comment.
Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @dzmitryhil, @miladz68, and @ysv)
integration-tests/modules/wasm_test.go line 2343 at r1 (raw file):
}) requireT.NoError(err) requireT.Equal("/cosmos.vesting.v1beta1.DelayedVestingAccount", accountRes.Account.TypeUrl)
I correctly remembered that there is a reason for treating vesting accounts differently. this test fails:
Error Trace: /home/wojciech/coreum/integration-tests/modules/wasm_test.go:2343
Error: Not equal:
expected: "/cosmos.vesting.v1beta1.DelayedVestingAccount"
actual : "/cosmos.auth.v1beta1.BaseAccount"
Diff:
--- Expected
+++ Actual
@@ -1 +1 @@
-/cosmos.vesting.v1beta1.DelayedVestingAccount
+/cosmos.auth.v1beta1.BaseAccount
Test: TestVestingToWASMContract
--- FAIL: TestVestingToWASMContract (4.27s)
There was a problem hiding this comment.
Reviewable status: 0 of 3 files reviewed, all discussions resolved (waiting on @dzmitryhil, @miladz68, and @ysv)
integration-tests/modules/wasm_test.go line 2343 at r1 (raw file):
Previously, wojtek-coreum (Wojtek) wrote…
I correctly remembered that there is a reason for treating vesting accounts differently. this test fails:
Error Trace: /home/wojciech/coreum/integration-tests/modules/wasm_test.go:2343 Error: Not equal: expected: "/cosmos.vesting.v1beta1.DelayedVestingAccount" actual : "/cosmos.auth.v1beta1.BaseAccount" Diff: --- Expected +++ Actual @@ -1 +1 @@ -/cosmos.vesting.v1beta1.DelayedVestingAccount +/cosmos.auth.v1beta1.BaseAccount Test: TestVestingToWASMContract --- FAIL: TestVestingToWASMContract (4.27s)
Okay, I found a way to overcome this
There was a problem hiding this comment.
Reviewed 1 of 2 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @dzmitryhil, @miladz68, and @wojtek-coreum)
integration-tests/modules/wasm_test.go line 2152 at r2 (raw file):
requireT.NoError(err) contract, err := chain.Wasm.PredictWASMContractAddress(
is it safe to run this test in parallel with other ?
Because if we instantiate contract in a different test we might have wrong prediction ?
integration-tests/modules/wasm_test.go line 2165 at r2 (raw file):
FromAddress: admin.String(), ToAddress: contract.String(), Amount: sdk.NewCoins(amount),
I suggest to send different amounts to different addresses
this will make test less error prone
integration-tests/modules/wasm_test.go line 2185 at r2 (raw file):
}, ) requireT.NoError(err)
I think we should assert that our predicted smart contract address is equal to actuall
Otherwise there is no reason to continue tests
requireT.Equal(contractAddr1, contract)
same applies to another places
integration-tests/modules/wasm_test.go line 2185 at r2 (raw file):
}, ) requireT.NoError(err)
shouldn't we check if balance is equal to some value after instantiation ?
integration-tests/modules/wasm_test.go line 2407 at r2 (raw file):
}) requireT.NoError(err) requireT.Equal("/cosmos.vesting.v1beta1.DelayedVestingAccount", accountRes.Account.TypeUrl)
is there a different acc type for contract ? or they are just auth.BaseAccount ?
integration-tests/modules/wasm_test.go line 2407 at r2 (raw file):
are we sure it is auth.BaseAccount ?
wasm code:
type AccountPruner interface {
// CleanupExistingAccount handles the cleanup process for balances and data of the given account. The persisted account// type is already reset to base account at this stage.
// The method returns true when the account address can be reused. Unsupported account types are rejected by returning false
CleanupExistingAccount(ctx sdk.Context, existingAccount authtypes.AccountI) (handled bool, err error)
}
type is already reset to base account at this stage
integration-tests/modules/wasm_test.go line 2434 at r2 (raw file):
admin, contractAddr, moduleswasm.BankSendExecuteWithdrawRequest(amount, recipient),
so funds are fully vested after 30s ?
But another thing I wanted to check is that funds are not vested before vesting period passes. Even though the contract is instantiated
Vector of attack.
- airdrop creates vesting accounts
- you provide contract address as receiver
- once received you instantiate contract
- funds are unlocked
x/wasm/types/prunner.go line 14 at r2 (raw file):
// CleanupExistingAccount informs wasm module to reject smart contract instantiation if account exists. func (ap AccountPruner) CleanupExistingAccount(_ sdk.Context, _ authtypes.AccountI) (bool, error) { return true, nil
did we find the reason why they burn vesting accounts ?
I think there was a purpose for this
There was a problem hiding this comment.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @miladz68 and @wojtek-coreum)
app/app.go line 596 at r2 (raw file):
&authtypes.BaseAccount{}, &vestingtypes.ContinuousVestingAccount{}, &vestingtypes.DelayedVestingAccount{},
What about periodic?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @dzmitryhil, @miladz68, and @ysv)
app/app.go line 596 at r2 (raw file):
Previously, dzmitryhil (Dzmitry Hil) wrote…
What about periodic?
Added, but without test because this account might only be created in genesis
integration-tests/modules/wasm_test.go line 2152 at r2 (raw file):
Previously, ysv (Yaroslav Savchuk) wrote…
is it safe to run this test in parallel with other ?
Because if we instantiate contract in a different test we might have wrong prediction ?
It is safe because it uses Instantiate2 - there is no global counter
integration-tests/modules/wasm_test.go line 2165 at r2 (raw file):
Previously, ysv (Yaroslav Savchuk) wrote…
I suggest to send different amounts to different addresses
this will make test less error prone
Done.
integration-tests/modules/wasm_test.go line 2185 at r2 (raw file):
Previously, ysv (Yaroslav Savchuk) wrote…
I think we should assert that our predicted smart contract address is equal to actuall
Otherwise there is no reason to continue tests
requireT.Equal(contractAddr1, contract)same applies to another places
Done.
integration-tests/modules/wasm_test.go line 2185 at r2 (raw file):
Previously, ysv (Yaroslav Savchuk) wrote…
shouldn't we check if balance is equal to some value after instantiation ?
Done.
integration-tests/modules/wasm_test.go line 2407 at r2 (raw file):
Previously, ysv (Yaroslav Savchuk) wrote…
is there a different acc type for contract ? or they are just
auth.BaseAccount?
BaseAccount
integration-tests/modules/wasm_test.go line 2407 at r2 (raw file):
Previously, ysv (Yaroslav Savchuk) wrote…
are we sure it is auth.BaseAccount ?
wasm code:
type AccountPruner interface { // CleanupExistingAccount handles the cleanup process for balances and data of the given account. The persisted account// type is already reset to base account at this stage. // The method returns true when the account address can be reused. Unsupported account types are rejected by returning false CleanupExistingAccount(ctx sdk.Context, existingAccount authtypes.AccountI) (handled bool, err error) }type is already reset to base account at this stage
yes, it is. Added check fr this.
integration-tests/modules/wasm_test.go line 2434 at r2 (raw file):
Previously, ysv (Yaroslav Savchuk) wrote…
so funds are fully vested after 30s ?
But another thing I wanted to check is that funds are not vested before vesting period passes. Even though the contract is instantiated
Vector of attack.
- airdrop creates vesting accounts
- you provide contract address as receiver
- once received you instantiate contract
- funds are unlocked
Added check for this, all is as expected
There was a problem hiding this comment.
Reviewable status: 1 of 3 files reviewed, 9 unresolved discussions (waiting on @dzmitryhil, @miladz68, and @ysv)
x/wasm/types/prunner.go line 14 at r2 (raw file):
Previously, ysv (Yaroslav Savchuk) wrote…
did we find the reason why they burn vesting accounts ?
I think there was a purpose for this
Original burning was introduced here: CosmWasm/wasmd#996
and thn reimplemented here: CosmWasm/wasmd#1003
The only sentence explaining motivation is this:
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.
|
Closed in #740 |
Description
Reviewers checklist:
Authors checklist
This change is