[pyth-cosmwasm] sec review update #649
Conversation
0xfirefist
requested review from
ali-behjati and
jayantk
and removed request for
jayantk
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ |
| #[derive(Serialize, Deserialize, Clone, Debug, PartialEq, JsonSchema)] | ||
| pub struct ConfigInfo { | ||
| pub owner: Addr, | ||
| // TODO: ASK do we need to update config key? |
There was a problem hiding this comment.
What do you mean here?
There was a problem hiding this comment.
Before, the config had owner in it and now it doesn't
Should we update the config key use to store the config?
Intuitively I think it won't matter. As, all the fields required will still be there. But not sure.
There was a problem hiding this comment.
I don't know to what format Cosmwasm serialized though but I think it will break. We can update the config and have proper migration or rename it to deprecated_owner and keep it. @njk-64 what do you think?
There was a problem hiding this comment.
Yeah this will probably break.
Actually, this brings up a good question, which is how are we going to manage changes to this config class going forward?
I think we probably want to (1) stick a version number field in ConfigInfo so we can implement hard migration logic, and (2) add #repr[(C)] so we can extend it with fields at the end without having rust be too smart about packing the fields of the struct and potentially reordering things.
There was a problem hiding this comment.
I don't think that Serialize and Deserialize work with the way rust stores data on memory. But anyway migration is not very difficult on the migrate method when we need it.
jayantk
left a comment
jayantk
left a comment
There was a problem hiding this comment.
LGTM. we should deal with the config issue also. will slack you.
| #[derive(Serialize, Deserialize, Clone, Debug, PartialEq, JsonSchema)] | ||
| pub struct ConfigInfo { | ||
| pub owner: Addr, | ||
| // TODO: ASK do we need to update config key? |
There was a problem hiding this comment.
Yeah this will probably break.
Actually, this brings up a good question, which is how are we going to manage changes to this config class going forward?
I think we probably want to (1) stick a version number field in ConfigInfo so we can implement hard migration logic, and (2) add #repr[(C)] so we can extend it with fields at the end without having rust be too smart about packing the fields of the struct and potentially reordering things.
address the feedback for the initial sec review
following in particular: