-
Notifications
You must be signed in to change notification settings - Fork 6
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
v2 #11
Conversation
…adata Allow custom LP subdenom and setting denom metadata
|
||
/// get the alloyed denom | ||
pub fn get_alloyed_denom(&self, store: &dyn Storage) -> StdResult<String> { | ||
self.alloyed_denom.load(store) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I generally like this pattern, but I'm thinking that it may lead to loads happening multiple times if the caller uses several get methods.
Not release-blocker, but we can consider caching these (if the data is there return it, otherwise fetch)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice idea! on it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(just remember to reset the cache on sets! :p )
contracts/transmuter/src/contract.rs
Outdated
@@ -128,13 +346,44 @@ impl Transmuter<'_> { | |||
&self, | |||
ctx: (DepsMut, Env, MessageInfo), | |||
tokens_out: Vec<Coin>, | |||
) -> Result<Response, ContractError> { | |||
self.swap_alloyed_asset_for_tokens("exit_pool", ctx, tokens_out) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These being join/exit pool confused me at first, but I get it now
contracts/transmuter/src/contract.rs
Outdated
let alloyed_denom = self.alloyed_asset.get_alloyed_denom(deps.storage)?; | ||
|
||
// if funds contains one coin and is alloyed asset, use that as sender's share | ||
let (sender_shares, burn_from_address) = if info.funds.is_empty() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain these two blocks of code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ohh, this is what makes it work as both exit_pool and swap. Maybe just add some comments for each block
contracts/transmuter/src/contract.rs
Outdated
// --- admin --- | ||
|
||
#[msg(exec)] | ||
pub fn transfer_admin( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not transmuter specific, but there are a few patterns that we keep using. It would be good to make sylvia interfaces that we can reuse on all osmosis contracts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, I think this is simlilar to openzeppelin Ownable, we might wanna have our own collection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
100%! we could even add some osmosis-specific stuff behind compilation flags (so they're only included when needed)
V2 issue tracker: #5
included PRs