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.

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

Add a coins_to_string helper #1359

Closed
maurolacy opened this issue Jul 13, 2022 · 2 comments · Fixed by confio/poe-contracts#174
Closed

Add a coins_to_string helper #1359

maurolacy opened this issue Jul 13, 2022 · 2 comments · Fixed by confio/poe-contracts#174

Comments

@maurolacy
Copy link
Contributor

Move coins_to_string
https://github.com/confio/poe-contracts/blob/ab1b1b21d235ee08419a158e2f7b26be8057ec9d/contracts/tg4-stake/src/contract.rs#L478-L485

to cosmwasm-std (src/coins.rs).

@webmaster128
Copy link
Member

webmaster128 commented Jul 14, 2022

I'm not super convinced we need this or should implement it here. In Cosmos SDK there is a dedicated Coins type which makes it easy to create helpers based on that. But lists of coins can create all sort of trouble, especially when it comes to duplicates and sorting. Creating coins_to_string opens more problems than it solves I think.

But I see two ways to improve the situation in the linked code:

  1. A Coin has a display implementation that explicitely uses the Cosmos SDK formatting format!("{}{}", c.amount, c.denom).
  2. The code only uses one coin

For this reason I think this is the more pragmatic and maybe even better solution:

diff --git a/contracts/tg4-stake/src/contract.rs b/contracts/tg4-stake/src/contract.rs
index 848f6b8..2f44b2a 100644
--- a/contracts/tg4-stake/src/contract.rs
+++ b/contracts/tg4-stake/src/contract.rs
@@ -461,29 +461,20 @@ pub fn execute_claim<Q: CustomQuery>(
     }
 
     let config = CONFIG.load(deps.storage)?;
-    let amount = coins(release.into(), config.denom);
+    let amount = coin(release.into(), config.denom);
 
     let res = Response::new()
         .add_attribute("action", "claim")
-        .add_attribute("tokens", coins_to_string(&amount))
+        .add_attribute("tokens", amount.to_string())
         .add_attribute("sender", &info.sender)
         .add_message(BankMsg::Send {
             to_address: info.sender.into(),
-            amount,
+            amount: vec![amount],
         });
 
     Ok(res)
 }
 
-// TODO: put in cosmwasm-std (https://github.com/CosmWasm/cosmwasm/issues/1359)
-fn coins_to_string(coins: &[Coin]) -> String {
-    let strings: Vec<_> = coins
-        .iter()
-        .map(|c| format!("{}{}", c.amount, c.denom))
-        .collect();
-    strings.join(",")
-}
-
 #[cfg_attr(not(feature = "library"), entry_point)]
 pub fn sudo(
     deps: DepsMut<TgradeQuery>,

@webmaster128
Copy link
Member

Closing in favour of #1377

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants