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.

Sign up for GitHub

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

Made admin field optional in MsgInstantiateContract #115

Merged
merged 3 commits into from
Aug 4, 2021
Merged

Made admin field optional in MsgInstantiateContract #115

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
31df592
Made admin field optional in MsgInstantiateContract
jstuczyn Aug 4, 2021
20d9b10
Simplified admin parsing
jstuczyn Aug 4, 2021
45313ad
Reverting broken changes
jstuczyn Aug 4, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions cosmos-sdk-rs/src/cosmwasm.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ pub struct MsgInstantiateContract {
pub sender: AccountId,

/// Admin is an optional address that can execute migrations
pub admin: AccountId,
pub admin: Option<AccountId>,

/// CodeID is the reference to the stored WASM code
pub code_id: u64,
Expand Down Expand Up @@ -174,9 +174,14 @@ impl TryFrom<proto::cosmwasm::wasm::v1beta1::MsgInstantiateContract> for MsgInst
} else {
Some(proto.label)
};
let admin = if proto.admin.is_empty() {
None
} else {
Some(proto.admin.parse())
};
Comment on lines +177 to +181
Copy link
Member

Choose a reason for hiding this comment

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

Any reason you didn't write this as...

Suggested change
let admin = if proto.admin.is_empty() {
None
} else {
Some(proto.admin.parse())
};
let admin = proto.admin.map(|admin| admin.parse());

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Honestly, didn't think of it at the time, good catch! I've applied the suggestion in the next commit and moved it directly to field assignment to keep it all more compact.

Copy link
Member

Choose a reason for hiding this comment

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

Oh whoops, now I see why: it's a String, and you do want the original behavior. My bad.

Unfortunately we don't really have a good helper for that either. The closest thing is probably tendermint_proto::serializers::optional_from_str, although I wouldn't suggest using that.

Mind reverting? Sorry again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Absolutely. I'm also sorry for not properly checking the suggestion myself. I forgot to pass features=cosmwasm and assumed it was all fine...

Ok(MsgInstantiateContract {
sender: proto.sender.parse()?,
admin: proto.admin.parse()?,
admin: admin.transpose()?,
code_id: proto.code_id,
label,
init_msg: proto.init_msg,
Expand All @@ -193,7 +198,7 @@ impl From<MsgInstantiateContract> for proto::cosmwasm::wasm::v1beta1::MsgInstant
fn from(msg: MsgInstantiateContract) -> proto::cosmwasm::wasm::v1beta1::MsgInstantiateContract {
proto::cosmwasm::wasm::v1beta1::MsgInstantiateContract {
sender: msg.sender.to_string(),
admin: msg.admin.to_string(),
admin: msg.admin.map(|admin| admin.to_string()).unwrap_or_default(),
code_id: msg.code_id,
label: msg.label.unwrap_or_default(),
init_msg: msg.init_msg,
Expand Down