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

Feature/token overhaul #5

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Feature/token overhaul #5

wants to merge 7 commits into from

Conversation

FloppyDisck
Copy link
Owner

No description provided.

@FloppyDisck FloppyDisck marked this pull request as ready for review November 6, 2024 14:55
Copy link

@pyncz pyncz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm. Honestly, I was thinking of something more minimalistic... Like, implementing send, burn and probably balance (with owner_of query under the hood) with the same params as for Fungible trait, with only difference that amount is always 1 (or either 0 or 1 in case of balance), so this way it can be wrapped in some generic Token trait.

We would just create a list of tokens with something like this:

let tokens = vec![
  Token::cw721("addr", "token_id"),
  Token::native("denom", 100),
  Token::cw20("addr", 100)
];

...and then would be able to call those send methods for each, not caring about what is that token exactly.

for token in tokens {
  resp.add_message(token.send("recipient")?);
  // ...
}

Note that cw721's TokenInfo in this setup would require token_id as well, e.g.:

pub enum TokenInfo {
    Cw20(Cw20TokenInfo), // Cw20TokenInfo { address: Addr }
    Native(NativeTokenInfo), //  NativeTokenInfo { denom: String }
    Cw721(Cw721TokenInfo), //  Cw721TokenInfo { address: Addr, token_id: String }
}

Disadvantages:

  • kinda confusing, I only started thinking about it and there are already 2 wacky conditions (one with amount=0|1 and one with balance being in fact a wrapper for owner_of)
  • kinda subjective, because this way we'd implement some methods from cw721 standard but not all of them

But on the other side, the proposed wrapper methods (for tokens, total_tokens etc) where you just call a corresponding query under the hood, don't bring much value.

#[derive(Serialize, Deserialize, Clone, Debug, PartialEq, JsonSchema)]
pub struct NonFungibleToken {
pub info: NonFungibleTokenInfo,
pub token: String,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's token_id, right? I think it would be more clear to keep that name

@FloppyDisck
Copy link
Owner Author

Hm. Honestly, I was thinking of something more minimalistic... Like, implementing send, burn and probably balance (with owner_of query under the hood) with the same params as for Fungible trait, with only difference that amount is always 1 (or either 0 or 1 in case of balance), so this way it can be wrapped in some generic Token trait.

We would just create a list of tokens with something like this:

let tokens = vec![
  Token::cw721("addr", "token_id"),
  Token::native("denom", 100),
  Token::cw20("addr", 100)
];

How about the proposed Token that wraps over Fungible and NonFungibleToken there I try to unite as much as I can about the interfaces, the issue is that the only methods that can be united are execution methods, balance is very hard to abstract since you'd be expecting very different values from each one.

But on the other side, the proposed wrapper methods (for tokens, total_tokens etc) where you just call a corresponding query under the hood, don't bring much value.

These are mostly to avoid having to import the cw packages directly, keeping things easier to read SubMsg::new(token.send()) is faster than the whole boilerplate we already do.

@pyncz
Copy link

pyncz commented Nov 6, 2024

balance is very hard to abstract since you'd be expecting very different values from each one

Is it tho? I guess it would be quite possible to expect:

  • for native - balance amount, obviously;
  • for cw20 - balance amount, obviously;
  • for cw721 - 1 if the checked address owns that token, and 0 otherwise.

So it's always token.send(recipient_addr: Addr) -> Uint128

--

keeping things easier to read SubMsg::new(token.send()) is faster than the whole boilerplate we already do

I totally agree with that, I mentioned that send, burn and probably balance are exactly where they should be. However, more cw721 specific methods (like owner_of, num_tokens, tokens etc etc) don't necessary belong to this package imo. Like, if devs want not just abstraction, but to operate with cw721 API directly, they can always import those cw721 packages.

@FloppyDisck
Copy link
Owner Author

I totally agree with that, I mentioned that send, burn and probably balance are exactly where they should be. However, more cw721 specific methods (like owner_of, num_tokens, tokens etc etc) don't necessary belong to this package imo. Like, if devs want not just abstraction, but to operate with cw721 API directly, they can always import those cw721 packages.

I feel like the purpose of CosmosCoin is abstracting many of these things in an easy to consume structure. So keeping these functions help the concept. If you're operating the API directly, then what's the point of using NativeToken for example

@FloppyDisck
Copy link
Owner Author

balance is very hard to abstract since you'd be expecting very different values from each one

Is it tho? I guess it would be quite possible to expect:

  • for native - balance amount, obviously;
  • for cw20 - balance amount, obviously;
  • for cw721 - 1 if the checked address owns that token, and 0 otherwise.

So it's always token.send(recipient_addr: Addr) -> Uint128

From an interface perspective that's horrible UX, + keep in mind that balance in this perspective would mean cw721's owned tokens for that address, which is an array of strings

@FloppyDisck
Copy link
Owner Author

Also MB, I accidentally edited your comment, I didn't know I could do that lmao

@pyncz
Copy link

pyncz commented Nov 7, 2024

I feel like the purpose of CosmosCoin is abstracting many of these things in an easy to consume structure. So keeping these functions help the concept. If you're operating the API directly, then what's the point of using NativeToken for example

I see value in NativeToken mostly as part of that bigger Token abstraction, not something one uses directly to just replace specific original API (whether it's native calls, cw20 or cw721)... But alright, I don't mind those 1:1 wrappers ofc, if you find them useful.

@pyncz
Copy link

pyncz commented Nov 7, 2024

From an interface perspective that's horrible UX

If the purpose of the package is to provide different wrapper structs for those different kinds of tokens, then yeah, probably. But if the main value is abstraction that allowes to use just Token and have the same interfaces implemented for each kind, then this is the way.

keep in mind that balance in this perspective would mean cw721's owned tokens for that address, which is an array of strings

Again, it depends on how you view the CosmosCoin package. If it's a set of wrappers with simpler API, then yes, "balance" of cw721 (i.e. of a collection) is a list of token ids. If it's abstraction, then balance being user's balance of specific cw721 token (i.e. 1 if owned and 0 if not) sounds right.

I see you lean towards the first approach. It's fine since it's your lib ofc, if so, I will just implement another crate / local package that meets our specific needs.

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 this pull request may close these issues.

2 participants