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

feat: change design of get coins endpoint #51

Closed
wants to merge 2 commits into from
Closed

Conversation

telezhnaya
Copy link
Contributor

After a discussion with @pkudinov, we came up to the conclusion that we need to change the Coins interface a bit.

We constantly think about the changes that we need to do in the future. In particular, we need to keep in mind that we have Multi Tokens, and we should be able to show multiple balances for one contract.
Initially, we decided to have several items in the response for Multi Tokens.

That gives us the problem with the pagination: we need to create the solution for the case when MT is cut in the middle between 2 pages.

Here, we need to make a step back and remind the nature of MT. I can imagine a few use cases: swapping pools/exchanges, liquidity pools, gaming. For all of them, it makes no sense to show only 1 MT balance. The users (I mean wallet creators) will want to group them. In this case, let's just group them on our side.

With all the advantages of this approach, I see 1 disadvantage - for the majority of use cases, there will be the balances list with just one item inside. And balances looks weird when you ask for native/FT balance (why I can get more than 1 balance there). It looks like overengineering to me. But, I anyway vote for these changes.

@@ -90,14 +90,22 @@ pub struct FtContractMetadataResponse {
pub struct Coin {
/// "nearprotocol" for NEAR, "nep141" for FT
pub standard: String,
pub balance: types::U128,
/// For NEAR/FT, there is always 1 item in the list. For MT, there could be more
pub balances: Vec<BalanceItem>,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It could be Map<String, u128>, but I decided to be more explicit. In the current solution, the user knows the field names

@telezhnaya
Copy link
Contributor Author

Ah, 1 more thing, I haven't updated tests for a reason, we don't have enough data still, so we can't update the snapshots properly. I'll do that right at the moment when we collect enough

@telezhnaya
Copy link
Contributor Author

Another idea from @frol:
We may stop even wondering if we will show MTs at the same endpoints with FTs. Maybe, at this point, we see clearly enough that it's better to rename Coin to FT.
Pros: much easier to make the design for this
Cons: users will have to make 2 requests to collect FTs and MTs

Actually, when we will finally make MT support, we may consider making an umbrella endpoint. It would be much easier to design it then, when we have more use cases and some data collected.

@khorolets
Copy link
Member

@telezhnaya and I had a meaningful conversation where I got all the answers regarding all of this.

I see that the best decision at this point would be

We may stop even wondering if we will show MTs at the same endpoints with FTs. Maybe, at this point, we see clearly enough that it's better to rename Coin to FT.
Pros: much easier to make the design for this
Cons: users will have to make 2 requests to collect FTs and MTs

MT standard is still "in progress" and might change. So all our attempts to support it are based on a bunch of assumptions and it's just slowing us down.

Let's support only supportable stuff and handle other stuff when it actually becomes a thing.

cc @pkudinov

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