feat(wasm): add contract access control #151

whylee259 · 2021-04-27T04:54:42Z

Description

closes: #126

Add contract status control and execution rejection logic.

Add a permission type ContractStatusAccess which defines access control permission.
The default value is NoBody and it will be modified using gov
The ContractStatus is added in ContractInfo states.
And it has two options Active and Inactive (Unspecified option will be rejected)
Add a message MsgUpdateContractStatus for a qualified permission owner to modify the ContractStatus.
If the status Inactive, the contract call will be rejected. (Execution, Migration, UpdateAdmin, ClearAdmin)

#131 must be merged first

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer
Review Codecov Report in the comment section below once CI passes

- add an enum `ContractStatus` - add a field `status` into ContractInfo

- add a msg `MsgUpdateContractStatus` - add handler for it

codecov · 2021-04-27T07:18:17Z

Codecov Report

Merging #151 (6620bc5) into v2/develop (1f739c5) will decrease coverage by 1.93%.
The diff coverage is 68.12%.

@@              Coverage Diff               @@
##           v2/develop     #151      +/-   ##
==============================================
- Coverage       54.99%   53.06%   -1.94%     
==============================================
  Files             812      704     -108     
  Lines           51487    47274    -4213     
==============================================
- Hits            28317    25086    -3231     
+ Misses          20211    19629     -582     
+ Partials         2959     2559     -400

Impacted Files	Coverage Δ
client/grpc/tmservice/query.pb.gw.go	`30.06% <ø> (ø)`
client/keys/root.go	`100.00% <ø> (ø)`
client/keys/utils.go	`35.29% <ø> (-3.60%)`	⬇️
crypto/armor.go	`85.89% <ø> (ø)`
crypto/keys/ed25519/ed25519.go	`67.34% <ø> (ø)`
crypto/keys/multisig/codec.go	`100.00% <ø> (ø)`
crypto/keys/secp256k1/secp256k1.go	`86.79% <ø> (ø)`
simapp/test_helpers.go	`0.49% <0.00%> (-0.01%)`	⬇️
types/address.go	`65.71% <ø> (ø)`
types/config.go	`84.61% <ø> (ø)`
... and 80 more

brew0722 · 2021-04-28T05:05:17Z

I understood it to be used as Instantiate Auth.

However, There is a part that is suspected of being a problem in the planning that the entity with Instantiate authority can instantiate again the Contract that has made the decision to disuse it.

However, first of all, we are satisfied with the implementation of the planning prototype based on the address, so we do not care about this in this PR.

I understand Blocking/allowing standards for planning via CodeID(CodeInfo).
However, the implementation appears to be a ContractAddress(ContractInfo).

If it is disabled based on address, I think it seems that Instantiate cannot be controlled.

brew0722

LGTM

loloicci

Should not the status of the contract affect Queries?

loloicci · 2021-04-28T08:43:08Z

x/wasm/internal/keeper/authz_policy.go

@@ -40,3 +45,7 @@ func (p GovAuthorizationPolicy) CanInstantiateContract(types.AccessConfig, sdk.A
 func (p GovAuthorizationPolicy) CanModifyContract(sdk.AccAddress, sdk.AccAddress) bool {
 	return true
 }
+
+func (p GovAuthorizationPolicy) CanUpdateContractStatus(types.AccessConfig, sdk.AccAddress) bool {


When is this (and above 3 functions) called and what is this for? Want comment doc.

I have added the comment to the code.

loloicci · 2021-04-28T08:49:42Z

x/wasm/internal/keeper/genesis_test.go

@@ -469,7 +472,8 @@ func TestImportContractWithCodeHistoryReset(t *testing.T) {
        "code_id": "1",
        "creator": "link1p0yx9c9q4xsnedlcn24gqfry5dcu6e9xkhv9aj",
        "admin": "link1qyqszqgpqyqszqgpqyqszqgpqyqszqgp8apuk5",
-        "label": "ȀĴnZV芢毤"
+        "label": "ȀĴnZV芢毤",
+		"status": "Active"


Mixed Tab vs Space in these templates. Want to unify.

I have fixed it to use spaces.

- in function `TestImportContractWithCodeHistoryReset` of `genesis_test.go`

whylee259 · 2021-04-29T04:10:41Z

Should not the status of the contract affect Queries?

You mean that query responses should not include the status, right?
The original concept of mine was that the status is included for ContractInfo queries.

shiki-tak

LGTM

shiki-tak · 2021-04-30T07:13:47Z

x/wasm/internal/keeper/keeper.go

+	if contractInfo == nil {
+		return sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "unknown contract")
+	}
+	if contractInfo.Status != status {


Can status be nil?

No, it is a value type.
It can be ContractStatusUnspecified but it's not a problem.

whylee259 marked this pull request as draft April 27, 2021 04:55

whylee259 self-assigned this Apr 27, 2021

whylee259 added the C:WASM label Apr 27, 2021

whylee259 force-pushed the whylee/v2/feat/contract-access-control branch from 3b8b771 to 97d4c0d Compare April 27, 2021 06:34

Yongwoo Lee added 3 commits April 27, 2021 15:46

add permission param ContractStatusAccess

b620034

add ContractStatus type and field

90863fe

- add an enum `ContractStatus` - add a field `status` into ContractInfo

add msg MsgUpdateContractStatus

d766bfd

- add a msg `MsgUpdateContractStatus` - add handler for it

whylee259 force-pushed the whylee/v2/feat/contract-access-control branch from 97d4c0d to 5f2a0ce Compare April 27, 2021 06:48

Yongwoo Lee added 2 commits April 27, 2021 16:08

reject inactive smart contract call

1eca526

add changlog entry to CHANGELOG_PENDING.md

c6c6868

whylee259 force-pushed the whylee/v2/feat/contract-access-control branch from 5f2a0ce to c6c6868 Compare April 27, 2021 07:08

whylee259 marked this pull request as ready for review April 27, 2021 07:18

whylee259 requested review from brew0722, loloicci and shiki-tak April 27, 2021 07:19

brew0722 approved these changes Apr 28, 2021

View reviewed changes

loloicci reviewed Apr 28, 2021

View reviewed changes

Yongwoo Lee added 5 commits April 29, 2021 10:29

use spaces rather than tabs for json tamplate

7206053

- in function `TestImportContractWithCodeHistoryReset` of `genesis_test.go`

Add comments for GovAuthorizationPolicy

bb367d3

Add gov proposal for update contract status

b115e2c

Add proposal handler for updating contract status

397865b

Merge branch 'v2/develop' into whylee/v2/feat/contract-access-control

3d284fb

brew0722 self-requested a review April 29, 2021 04:15

brew0722 approved these changes Apr 29, 2021

View reviewed changes

shiki-tak approved these changes Apr 30, 2021

View reviewed changes

Yongwoo Lee added 2 commits April 30, 2021 17:19

Merge branch 'v2/develop' into whylee/v2/feat/contract-access-control

b6a7f1a

Merge branch 'v2/develop' into whylee/v2/feat/contract-access-control

6620bc5

whylee259 merged commit 62843eb into v2/develop May 3, 2021

whylee259 deleted the whylee/v2/feat/contract-access-control branch May 12, 2021 05:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(wasm): add contract access control #151

feat(wasm): add contract access control #151

whylee259 commented Apr 27, 2021 •

edited

Loading

codecov bot commented Apr 27, 2021 •

edited

Loading

brew0722 commented Apr 28, 2021 •

edited

Loading

brew0722 left a comment

loloicci left a comment

loloicci Apr 28, 2021

whylee259 Apr 29, 2021

loloicci Apr 28, 2021 •

edited

Loading

whylee259 Apr 29, 2021

whylee259 commented Apr 29, 2021

shiki-tak left a comment

shiki-tak Apr 30, 2021

whylee259 Apr 30, 2021 •

edited

Loading

feat(wasm): add contract access control #151

feat(wasm): add contract access control #151

Conversation

whylee259 commented Apr 27, 2021 • edited Loading

Description

codecov bot commented Apr 27, 2021 • edited Loading

Codecov Report

brew0722 commented Apr 28, 2021 • edited Loading

brew0722 left a comment

Choose a reason for hiding this comment

loloicci left a comment

Choose a reason for hiding this comment

loloicci Apr 28, 2021

Choose a reason for hiding this comment

whylee259 Apr 29, 2021

Choose a reason for hiding this comment

loloicci Apr 28, 2021 • edited Loading

Choose a reason for hiding this comment

whylee259 Apr 29, 2021

Choose a reason for hiding this comment

whylee259 commented Apr 29, 2021

shiki-tak left a comment

Choose a reason for hiding this comment

shiki-tak Apr 30, 2021

Choose a reason for hiding this comment

whylee259 Apr 30, 2021 • edited Loading

Choose a reason for hiding this comment

whylee259 commented Apr 27, 2021 •

edited

Loading

codecov bot commented Apr 27, 2021 •

edited

Loading

brew0722 commented Apr 28, 2021 •

edited

Loading

loloicci Apr 28, 2021 •

edited

Loading

whylee259 Apr 30, 2021 •

edited

Loading