Marketing info for cw20-base contract #375
Conversation
hashedone
force-pushed
the
371-cw20-logo
branch
from
2831cf6
to
a70894c
Compare
contracts/cw20-base/src/state.rs
Outdated
| pub project: Option<String>, | ||
| pub description: Option<String>, | ||
| pub marketing: Option<Addr>, | ||
| pub logo: Option<Logo>, |
There was a problem hiding this comment.
I was wondering about this. My idea was that the embedded logos will be huge and should be stored separately.
Thus, just LogoInfo there (which is url or a simple enum).
And if there is logo data, store the Logo in it's own map (which is only ready on DownloadLogo call.
There was a problem hiding this comment.
I really don't want to bloat up the TokenInfo struct with 5kb of SVG which must be loaded and parse every time.
There was a problem hiding this comment.
You are right - I didn't think about it. Actually as first instinct I stored whole MarketingInfo in separate Item, but went this way, but having Logo in separated place makes sense, I will refactor.
There was a problem hiding this comment.
On reflection, I think TokenInfo is updated on minting and consulted a number of places.
It may make sense to store TokenInfo and MarketingInfo separately. And Logo in a 3rd section (which may well be empty, use may_load to check it)
There was a problem hiding this comment.
But pulling out Logo is the main issue
contracts/cw20-base/src/msg.rs
Outdated
| @@ -107,6 +116,19 @@ pub enum ExecuteMsg { | |||
| }, | |||
| /// Only with "approval" extension. Destroys tokens forever | |||
| BurnFrom { owner: String, amount: Uint128 }, | |||
| /// Only with the "marketing" extension. If authorised, updates marketing metadata. | |||
There was a problem hiding this comment.
We could also use type ExecuteMsg = cw20::ExecuteMsg if you think that is clearer.
They should be identical and this was brought up sometime ago. Not necessary, but feel free to do that if you think it helps.
There was a problem hiding this comment.
I think it is better solution, I was surprised those types are separated. However I see downside of this solution - you cannot update cw20 without updating contract itself (as matches arm would not be fully covered). However it can be easly solved, just adding additional arm returning "Err::Std("Not yet implemented") or even just panicking with unimplemented!().
There was a problem hiding this comment.
Yeah, for cw20-base, which should be a reference implementation, I think it does make sense to error if we don't implement everything (or implement anything out of spec). Only instantiation is custom.
| impl TokenInfo { | ||
| pub fn get_cap(&self) -> Option<Uint128> { | ||
| self.mint.as_ref().and_then(|v| v.cap) | ||
| } | ||
| } | ||
|
|
||
| pub const TOKEN_INFO: Item<TokenInfo> = Item::new("token_info"); | ||
| pub const LOGO: Item<Logo> = Item::new("logo"); |
contracts/cw20-base/src/contract.rs
Outdated
| fn verify_logo(logo: &Logo) -> Result<(), ContractError> { | ||
| if matches!( | ||
| logo, | ||
| Logo::Embedded(EmbeddedLogo::Svg(logo)) |
There was a problem hiding this comment.
It would good to also verify the format. At least somewhat...
eg. svg starts with <?xml and ends with </svg> (or a slightly less restrictive contains search on this). Maybe some other items
png should at least have the magic number (4 bytes prefix).
We don't need to protect against people seeing malformed data, but rather check if they tried to shove a gif as a png. Better to error on upload.
There was a problem hiding this comment.
I was considering it, but abandoned as I decided that I am not able to perform validation in reasonable time (in terms of performance, not development time), and don't see point of such non-whole checks. In particular for svg to have reasonable check we should parse xml, as at the and </svg> doesn't need to be at the end (even comments may follow it), on the other hand - searching for </svg> may end up finding one which is specifically in the comment. Basically if someone would like to use malformed image as attack vector on someone using logo API, then we cannot protect it. The most we can actually do in reasonable time is checking xml preamble (as it shouldn't contain anything before it including comments), and probably header for png. I can add it.
contracts/cw20-base/src/contract.rs
Outdated
| @@ -47,13 +65,27 @@ pub fn instantiate( | |||
| None => None, | |||
| }; | |||
|
|
|||
| let marketing = match msg.marketing { | |||
There was a problem hiding this comment.
Logo cannot be set on instantiate, correct, only later?
There was a problem hiding this comment.
This belongs in a Rustdoc (so the js devs see it as well)
There was a problem hiding this comment.
Yea, after all I am not sure why I did remove logo from instantiation. I though about not making the data big, but it would be big only if someone sends such logo so I don't see actual issue. I think I will bring it back in instantiation.
contracts/cw20-base/src/contract.rs
Outdated
|
|
||
| // If there is no marketing info, there is also noone who can update it, therefore it cannot be | ||
| // authorised | ||
| let marketing_info = config |
There was a problem hiding this comment.
This method is proper code. I wonder if there is a simpler way to handle it?
| } | ||
|
|
||
| match marketing { | ||
| Some(empty) if empty.trim().is_empty() => marketing_info.marketing = None, |
There was a problem hiding this comment.
I never saw if statements on the branches of a match.
I had to look twice, but this is an interesting approach.
I would have just matched on Some(marketing) and done the if inside the branch. But this works too and is a bit shorter
| Ok(info) | ||
| } | ||
|
|
||
| pub fn query_download_logo(deps: Deps) -> StdResult<DownloadLogoResponse> { |
contracts/cw20-base/src/contract.rs
Outdated
| marketing: marketing.marketing, | ||
| logo: marketing.logo, | ||
| }, | ||
| None => MarketingInfoResponse { |
There was a problem hiding this comment.
MarketingInfoResponse::default() ?
Closes #371
Implementation of #370