Made admin field optional in MsgInstantiateContract #115
Conversation
| let admin = if proto.admin.is_empty() { | ||
| None | ||
| } else { | ||
| Some(proto.admin.parse()) | ||
| }; |
There was a problem hiding this comment.
Any reason you didn't write this as...
| let admin = if proto.admin.is_empty() { | |
| None | |
| } else { | |
| Some(proto.admin.parse()) | |
| }; | |
| let admin = proto.admin.map(|admin| admin.parse()); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Absolutely. I'm also sorry for not properly checking the suggestion myself. I forgot to pass features=cosmwasm and assumed it was all fine...
jstuczyn
force-pushed
the
jstuczyn/bugfix/optional-cosmwasm-admin
branch
from
4f72f7e to
20d9b10
Compare
I've run into my own omission from the previous pull request - as the comment suggests, the admin field should be optional.