-
Notifications
You must be signed in to change notification settings - Fork 28
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
Stop rejecting contract instantiation if account holds funds #776
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #776 +/- ##
==========================================
+ Coverage 33.04% 33.06% +0.01%
==========================================
Files 170 171 +1
Lines 48255 48266 +11
==========================================
+ Hits 15948 15957 +9
- Misses 29278 29280 +2
Partials 3029 3029 ☔ View full report in Codecov by Sentry. |
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.
Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @miladz68, @wojtek-coreum, and @ysv)
integration-tests/modules/wasm_test.go
line 2238 at r1 (raw file):
} // TestWASMContractInstantiationIsRejectedIfThereAreTokensOnItsAccount verifies that smart contract instantiation
TestWASMContractInstantiationIsRejectedIfThereAreTokensOnItsAccount
is different from the test name.
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.
Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @dzmitryhil, @miladz68, and @ysv)
integration-tests/modules/wasm_test.go
line 2238 at r1 (raw file):
Previously, dzmitryhil (Dzmitry Hil) wrote…
TestWASMContractInstantiationIsRejectedIfThereAreTokensOnItsAccount
is different from the test name.
Done, funny that linter passed
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.
Reviewable status: 0 of 3 files reviewed, all discussions resolved (waiting on @miladz68 and @ysv)
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.
Reviewed 2 of 3 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @miladz68 and @wojtek-coreum)
app/app.go
line 614 at r2 (raw file):
wasmOpts := []wasmkeeper.Option{ wasmkeeper.WithAcceptedAccountTypesOnContractInstantiation( &authtypes.BaseAccount{},
out of topic:
What happens when you try to instantiate contract with the same address as existing ?
Does This Pruner handle it ?
integration-tests/modules/wasm_test.go
line 2288 at r2 (raw file):
ToAddress: contractAddress.String(), Amount: sdk.NewCoins(amount), EndTime: time.Now().Unix(),
IMO endTime should be in the future to properly test that vesting is applied
integration-tests/modules/wasm_test.go
line 2302 at r2 (raw file):
ToAddress: contractAddress.String(), Amount: sdk.NewCoins(amount), EndTime: time.Now().Unix(),
IMO endTime should be in the future to properly test that vesting is applied
integration-tests/modules/wasm_test.go
line 2481 at r2 (raw file):
} func testSmartContractAccount(
what is the responsibility of this func ?
I didn't get the name
Code quote:
testSmartContractAccount
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.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @miladz68 and @ysv)
app/app.go
line 614 at r2 (raw file):
Previously, ysv (Yaroslav Savchuk) wrote…
out of topic:
What happens when you try to instantiate contract with the same address as existing ?
Does This Pruner handle it ?
I believe error saying that address is already taken is returned
integration-tests/modules/wasm_test.go
line 2288 at r2 (raw file):
Previously, ysv (Yaroslav Savchuk) wrote…
IMO endTime should be in the future to properly test that vesting is applied
Here I test that funds existing on the account are not burnt, that's why they are vested immediately. What you are describing is done separately in the next test below.
integration-tests/modules/wasm_test.go
line 2481 at r2 (raw file):
Previously, ysv (Yaroslav Savchuk) wrote…
what is the responsibility of this func ?
I didn't get the name
It verifies that the account used by the smart contract is in the expected state (account type, balance)
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.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @miladz68)
Description
Reviewers checklist:
Authors checklist
This change is