Skip to content
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

Add msg update contract label #1640

Merged
merged 2 commits into from
Sep 28, 2023
Merged

Add msg update contract label #1640

merged 2 commits into from
Sep 28, 2023

Conversation

pinosu
Copy link
Contributor

@pinosu pinosu commented Sep 27, 2023

Resolves #1601

@pinosu pinosu requested a review from alpe as a code owner September 27, 2023 12:56
@codecov
Copy link

codecov bot commented Sep 27, 2023

Codecov Report

Merging #1640 (8dd324f) into main (af8c491) will decrease coverage by 0.07%.
The diff coverage is 46.96%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1640      +/-   ##
==========================================
- Coverage   56.03%   55.96%   -0.07%     
==========================================
  Files          64       64              
  Lines        8996     9062      +66     
==========================================
+ Hits         5041     5072      +31     
- Misses       3514     3546      +32     
- Partials      441      444       +3     
Files Coverage Δ
x/wasm/keeper/keeper.go 80.02% <100.00%> (+0.42%) ⬆️
x/wasm/types/events.go 0.00% <ø> (ø)
x/wasm/client/cli/tx.go 15.18% <0.00%> (-0.04%) ⬇️
x/wasm/types/codec.go 58.82% <50.00%> (-0.18%) ⬇️
x/wasm/keeper/msg_server.go 63.09% <53.84%> (-0.47%) ⬇️
x/wasm/types/tx.go 51.82% <47.05%> (-0.23%) ⬇️
x/wasm/client/cli/new_tx.go 0.00% <0.00%> (ø)

Copy link
Contributor

@alpe alpe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work. 🎸
Only minor nits. Feel free to merge when you are happy with the code.

I would add the backport label to the PR but the backport will need some work from you, too

@@ -157,3 +157,31 @@ func UpdateInstantiateConfigCmd() *cobra.Command {
flags.AddTxFlagsToCmd(cmd)
return cmd
}

// UpdateContractLabelCmd sets an new label for a contract
func UpdateContractLabelCmd() *cobra.Command {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

types.EventTypeUpdateContractLabel,
sdk.NewAttribute(types.AttributeKeyContractAddr, contractAddress.String()),
sdk.NewAttribute(types.AttributeKeyNewLabel, newLabel),
))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

"update label - gov policy": {
newLabel: "new label",
policy: GovAuthorizationPolicy{},
caller: example.CreatorAddr,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When you use a random caller address, then it would ensure the different behaviour of the gov policy

assert.Equal(t, exp, attrsToStringMap(em.Events()[0].Attributes))
})
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good tests! For bonus points, there is also the unknown contract address scenario that must not panic.

return nil, err
}

return &types.MsgUpdateContractLabelResponse{}, nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

straight forward 👍

addr: myAddress.String(),
expErr: false,
},
"other address cannot update contract label": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 good to have the scenarios covered but there is some overlap with the keeper tests.
The integration tests give more confidence on the whole process. If you see value in both, then we can keep them.

addr: otherAddr.String(),
expErr: true,
},
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check for invalid data

}
})
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice test!

@@ -79,6 +80,7 @@ func RegisterInterfaces(registry types.InterfaceRegistry) {
&MsgAddCodeUploadParamsAddresses{},
&MsgRemoveCodeUploadParamsAddresses{},
&MsgStoreAndMigrateContract{},
&MsgUpdateContractLabel{},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important!

expErr: true,
},
}
for msg, spec := range specs {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellet!

@alpe alpe added the backport/v0.4x backport patches to sdk47 release branch label Sep 28, 2023
@pinosu pinosu merged commit e654808 into main Sep 28, 2023
7 of 8 checks passed
mergify bot pushed a commit that referenced this pull request Sep 28, 2023
* Add msg update contract label

* fix tests

(cherry picked from commit e654808)

# Conflicts:
#	x/wasm/keeper/keeper.go
#	x/wasm/types/tx.pb.go
pinosu added a commit that referenced this pull request Sep 28, 2023
* Add msg update contract label (#1640)

* Add msg update contract label

* fix tests

(cherry picked from commit e654808)

# Conflicts:
#	x/wasm/keeper/keeper.go
#	x/wasm/types/tx.pb.go

* Fix conflicts

---------

Co-authored-by: pinosu <95283998+pinosu@users.noreply.github.com>
Co-authored-by: Pino' Surace <pino.surace@live.it>
@pinosu pinosu deleted the 1601-add_update_label branch September 28, 2023 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/v0.4x backport patches to sdk47 release branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make contract lable updatable
2 participants