fix: delete unnecessary test #43

da1suk8 · 2023-05-02T08:01:29Z

Description

closes: #39

This test was moved from finschia-sdk by #1 and added by Finschia/finschia-sdk#470.

Previously, a function called UpdateContractStatus() was used in Finschia/finschia-sdk@bde56b9,
but it was removed from wasmd and has become a meaningless test.

Further checking with #48, active/inactive management is done by wasmplus.

wasmd/x/wasmplus/keeper/contract_keeper.go

Lines 15 to 16 in ff18c54

    
           activateContract(ctx sdk.Context, contractAddress sdk.AccAddress) error 
        
           deactivateContract(ctx sdk.Context, contractAddress sdk.AccAddress) error

In the test I deleted, "contract status", which was checked by UpdateContractStatus(), is not used in the current wasmd, so there is no problem in deleting it.

And when you use UpdateContractStatus() before, you could only change it via gov proposal, which was tested, but is no longer necessary.

So I think it is better to remove this test.

Motivation and context

I don't want to leave a meaningless test.

Checklist:

I followed the contributing guidelines and code of conduct.
I have added a relevant changelog to CHANGELOG.md
I have added tests to cover my changes.
I have updated the documentation accordingly.
I have updated API documentation client/docs/swagger-ui/swagger.yaml

loloicci

Is there a test doing as same as the test at Finschia/finschia-sdk@bde56b9 in x/wasmplus?

da1suk8 · 2023-05-08T05:39:24Z

I'll check.

da1suk8 · 2023-05-09T10:56:57Z

@loloicci

This test do almost the same thing.

wasmd/x/wasmplus/keeper/keeper_test.go

Lines 37 to 55 in 74de5b2

    
           func TestDeactivateContract(t *testing.T) { 
        
           	ctx, keepers := CreateTestInput(t, false, AvailableCapabilities) 
        
           	k := keepers.WasmKeeper 
        
           	example := InstantiateHackatomExampleContract(t, ctx, keepers) 
        
           	em := sdk.NewEventManager() 
        
           	// request no contract address -> fail 
        
           	err := k.deactivateContract(ctx, example.CreatorAddr) 
        
           	require.Error(t, err, fmt.Sprintf("no contract %s", example.CreatorAddr)) 
        
           	// success case 
        
           	err = k.deactivateContract(ctx, example.Contract) 
        
           	require.NoError(t, err) 
        
           	// already inactivate contract -> fail 
        
           	err = k.deactivateContract(ctx.WithEventManager(em), example.Contract) 
        
           	require.Error(t, err, fmt.Sprintf("already inactivate contract %s", example.Contract)) 
        
           }

Kynea0b

Is my understanding below correct?

The intent of the test is to execute an inactive contract and return an error, but the function UpdateContractStatus()
which was used in that case, has been eliminated. In addition, the test to confirm the execution of an inactive contract and the return of an error is performed by TestDeactivateContract. Therefore, the TestExecuteManualInactiveContractFailure() should be removed.

da1suk8 · 2023-05-10T03:15:40Z

@kokeshiM0chi
Yes, your understanding is correct.

loloicci · 2023-05-10T05:01:45Z

@da1suk8

This test do almost the same thing.

It looks not the same test. As bellow, the test you delete checks other ways than gov proposal cannot change the contract state. Finschia/finschia-sdk@bde56b9#diff-591082558a25fcf8aa75120ea3d175720c0293fbde4bdf526ee60b491466a570L823-L827

da1suk8 · 2023-05-10T06:02:51Z

@loloicci

I understand that UpdateContractStatus(), which was used in this function, is deleted in Finschia/finschia-sdk#625, and the handling of white list and black list is controlled by wasm (Finschia/finschia-sdk#655).
My understanding is not correct?

So I am thinking that the role of the test I said is the same.

loloicci · 2023-05-10T06:45:55Z

@da1suk8

My understanding is not correct?

I don't know because I cannot judge whether

is deleted in Finschia/finschia-sdk#625, and the handling of white list and black list is controlled by wasm (Finschia/finschia-sdk#655).

means the authorization to active/inactive contracts is not within the scope of x/wasmplus or not. We should confirm what part checks the authorization first.

In any case, there should be some tests checking authorization to active/inactive contracts works.

da1suk8 · 2023-05-10T07:15:41Z

@loloicci

I will respond to this PR after confirming it with another #48.

zemyblue

Please, update changelog.

da1suk8 · 2023-05-11T06:13:56Z

@zemyblue

Update CHANGELOG after this #48.
Thank you.

zemyblue · 2023-05-12T02:43:56Z

@zemyblue

Update CHANGELOG after this #48. Thank you.

No, please update changelog with each PR.

codecov · 2023-05-12T07:12:39Z

Codecov Report

Merging #43 (34e7c68) into main (514953c) will decrease coverage by 0.04%.
The diff coverage is 77.77%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #43      +/-   ##
==========================================
- Coverage   60.83%   60.79%   -0.04%     
==========================================
  Files          80       80              
  Lines        9879     9877       -2     
==========================================
- Hits         6010     6005       -5     
- Misses       3553     3555       +2     
- Partials      316      317       +1

Impacted Files	Coverage Δ
app/ante.go	`56.09% <ø> (ø)`
app/app.go	`88.01% <ø> (ø)`
app/encoding.go	`100.00% <ø> (ø)`
app/export.go	`12.30% <ø> (ø)`
app/test_access.go	`0.00% <ø> (ø)`
app/test_helpers.go	`0.64% <ø> (ø)`
appplus/app.go	`88.82% <ø> (ø)`
appplus/encoding.go	`100.00% <ø> (ø)`
appplus/export.go	`47.69% <ø> (ø)`
appplus/genesis.go	`100.00% <ø> (ø)`
... and 68 more

da1suk8 · 2023-05-12T07:13:26Z

@zemyblue

Added.

da1suk8 · 2023-05-15T08:41:34Z

As confirmed by #48, active/inactive management is done by wasmplus.

wasmd/x/wasmplus/keeper/contract_keeper.go

Lines 15 to 16 in ff18c54

    
           activateContract(ctx sdk.Context, contractAddress sdk.AccAddress) error 
        
           deactivateContract(ctx sdk.Context, contractAddress sdk.AccAddress) error

In the test I deleted, "contract status", which was checked by UpdateContractStatus(), is not used in the current wasmd, so there is no problem in deleting it.

And when you use UpdateContractStatus() before, you could only change it via gov proposal, which was tested, but is no longer necessary.

…#53) * build: replace line repositories with finschia repositories (#30) * build: replace line repositories with finschia repositories * Update x/wasmplus/README.md Co-authored-by: zemyblue <zemyblue@gmail.com> * Update x/wasm/ibc_reflect_test.go Co-authored-by: zemyblue <zemyblue@gmail.com> * Update .github/dependabot.yml Co-authored-by: jaeseung-bae <119839167+jaeseung-bae@users.noreply.github.com> * Update CHANGELOG.md Co-authored-by: jaeseung-bae <119839167+jaeseung-bae@users.noreply.github.com> * Update CHANGELOG.md Co-authored-by: jaeseung-bae <119839167+jaeseung-bae@users.noreply.github.com> * docs: replace a comment contains line with finschia one * build: replace line in Dockerfile with finschia * docs: replace line in README.md with Finschia * docs: update CHANGELOG --------- Co-authored-by: zemyblue <zemyblue@gmail.com> Co-authored-by: jaeseung-bae <119839167+jaeseung-bae@users.noreply.github.com> * chore: update changelog for release v0.1.3 (#31) * fix: stop wrap twice the response of handling non-plus wasm message in plus handler (#35) * fix: stop wrap twice non-plus wasm hander's response fix #33 * test: add tests handling non-plus wasm messages * docs: add this PR to CHANGELOG.md * fix: reflect golangci-lint * fix: simplify how to handle the message in wasmplus * chore: update notice (#44) * chore: update notice * chore: update changelog * fix: delete unnecessary test (#43) * fix: delete unnecessary test * docs: add CHANGELOG * feat: add admin-related events (#46) * add admin-related events in docs * fix ClearAdmin event * cherry-pick upstream admin-related events update * chore: update changelog for release v0.1.4 (#49) * build: update depending wasmvm to v1.1.1-0.11.2-dynamiclink2 and update tests --------- Co-authored-by: zemyblue <zemyblue@gmail.com> Co-authored-by: jaeseung-bae <119839167+jaeseung-bae@users.noreply.github.com> Co-authored-by: Daisuke Iuchi <42408108+da1suk8@users.noreply.github.com> Co-authored-by: Jayden Lee <41176085+tkxkd0159@users.noreply.github.com>

fix: delete unnecessary test

c7fc41a

da1suk8 marked this pull request as ready for review May 8, 2023 01:48

da1suk8 requested review from dudong2, loloicci and Kynea0b as code owners May 8, 2023 01:48

da1suk8 self-assigned this May 8, 2023

loloicci reviewed May 8, 2023

View reviewed changes

Kynea0b reviewed May 10, 2023

View reviewed changes

da1suk8 requested review from loloicci and Kynea0b May 10, 2023 04:03

Kynea0b previously approved these changes May 10, 2023

View reviewed changes

da1suk8 mentioned this pull request May 10, 2023

Check the authorization to active/inactive contracts is not within the scope of x/wasmplus or not #48

Closed

zemyblue reviewed May 11, 2023

View reviewed changes

docs: add CHANGELOG

34e7c68

da1suk8 dismissed Kynea0b’s stale review via 34e7c68 May 12, 2023 07:12

da1suk8 requested review from zemyblue and Kynea0b May 15, 2023 08:42

zemyblue approved these changes May 16, 2023

View reviewed changes

loloicci approved these changes May 16, 2023

View reviewed changes

da1suk8 merged commit 8518b2f into Finschia:main May 16, 2023

da1suk8 deleted the fix/delete_unnecessary_test branch May 16, 2023 02:38

This was referenced May 22, 2023

Release wasmd v0.1.4 #49

Merged

chore: bump up wasmd from v0.1.3 to v0.1.4 Finschia/finschia#191

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: delete unnecessary test #43

fix: delete unnecessary test #43

da1suk8 commented May 2, 2023 •

edited

Loading

loloicci left a comment

da1suk8 commented May 8, 2023

da1suk8 commented May 9, 2023

Kynea0b left a comment

da1suk8 commented May 10, 2023

loloicci commented May 10, 2023 •

edited

Loading

da1suk8 commented May 10, 2023 •

edited

Loading

loloicci commented May 10, 2023 •

edited

Loading

da1suk8 commented May 10, 2023 •

edited

Loading

zemyblue left a comment

da1suk8 commented May 11, 2023

zemyblue commented May 12, 2023

codecov bot commented May 12, 2023 •

edited

Loading

da1suk8 commented May 12, 2023

da1suk8 commented May 15, 2023 •

edited

Loading

	activateContract(ctx sdk.Context, contractAddress sdk.AccAddress) error
	deactivateContract(ctx sdk.Context, contractAddress sdk.AccAddress) error

fix: delete unnecessary test #43

fix: delete unnecessary test #43

Conversation

da1suk8 commented May 2, 2023 • edited Loading

Description

Motivation and context

Checklist:

loloicci left a comment

Choose a reason for hiding this comment

da1suk8 commented May 8, 2023

da1suk8 commented May 9, 2023

Kynea0b left a comment

Choose a reason for hiding this comment

da1suk8 commented May 10, 2023

loloicci commented May 10, 2023 • edited Loading

da1suk8 commented May 10, 2023 • edited Loading

loloicci commented May 10, 2023 • edited Loading

da1suk8 commented May 10, 2023 • edited Loading

zemyblue left a comment

Choose a reason for hiding this comment

da1suk8 commented May 11, 2023

zemyblue commented May 12, 2023

codecov bot commented May 12, 2023 • edited Loading

Codecov Report

da1suk8 commented May 12, 2023

da1suk8 commented May 15, 2023 • edited Loading

da1suk8 commented May 2, 2023 •

edited

Loading

loloicci commented May 10, 2023 •

edited

Loading

da1suk8 commented May 10, 2023 •

edited

Loading

loloicci commented May 10, 2023 •

edited

Loading

da1suk8 commented May 10, 2023 •

edited

Loading

codecov bot commented May 12, 2023 •

edited

Loading

da1suk8 commented May 15, 2023 •

edited

Loading