search

near / near-wallet

Web wallet for NEAR Protocol which stores keys in browser's localStorage
https://wallet.near.org
MIT License
220 stars 176 forks source link

View fungible token balances #1147

Closed kcole16 closed 3 years ago

kcole16 commented 3 years ago

Overview

The first story of #456 is enabling users to view their balances of NEP-21 tokens. The scope of this issue is limited to a token whitelist, which we should create.

Acceptance Criteria

Design

https://www.figma.com/file/MOMOnm89esHGC4PMKDBEDw/Fungible-Token-Balances?node-id=0%3A1

Screen Shot 2021-02-12 at 12 35 33 PM
heycorwin commented 3 years ago

@vgrichina could you post the simple schema for tokens and their contracts that we had talked about? Just need to get a basic sense of hierarchy and parent-child relationship between tokens and their contracts to inform how they might be displayed in the UI.

heycorwin commented 3 years ago

Following is a summary of the proposed initial design changes for the wallet. Note that while the changes being proposed in direct relation to the need to display fungible token balances are few, there are other high level changes being proposed that occur downstream (eg navigation / information architecture). These changes act as the first steps towards improving the overall information architecture of the wallet in order to be more accommodating of upcoming functionality around portfolio and asset management. Additional tweaks are suggested to the UI to make navigating through and interacting with the wallet and it's core feature set a more user friendly experience.

Summary

For the first iteration of displaying balances of fungible tokens in the wallet, the aim is to keep the feature-set small, and make improvements across the board in preparation for future iterations where functionality around managing your assets becomes more robust. This iteration's changes regarding fungible token balances are limited to their display. At this point, a user can only view the balances of their fungible tokens, and cannot interact with them in the wallet. Additional functionality will follow once these changes are in place.

The primary proposed changes are as follows:

  1. Display of fungible token "total" balances on /summary
  2. Minor redesign of summary to highlight NEAR as the primary asset, with other tokens being labeled as secondary.
  3. Moving Send and Receive to the main wallet view in order to highlight them as core wallet functions, and paving the way for additional functions to be added in a scalable manner.
  4. Relocation of the account activity feed to its own section of the app, in order to pave the way for the "Summary" view to better accommodate features more specific to asset and portfolio management.
  5. Small refactor of the wallet navigation to account for the relocation of the activity screen. Rename "Summary" to "Wallet", reflecting standard industry practice, and to reflect the fact that that is where the majority of asset mgmt will occur.
  6. Rename "Profile" to "Account Settings".
  7. Relocation of "Approved Apps" from /summary to "Account Settings". Moving forward, the primary function of "Account Settings" will be to house functions related to security, authentication, and other admin functions not related to core wallet features.
  8. Small refactor to how activity items are displayed in order to make them easier to consume and scan.
Screen Shot 2021-02-08 at 2 06 37 PM

Looking Ahead

  1. As a partner project to displaying balances, there is an opportunity to add a view for each fungible token that acts as a summary of that token's metadata. @vgrichina has modeled what a set of token metadata might look like here. At some point, it would benefit the user to be able to click on a particular asset (eg clicking "BNNA" would open a view of that tokens metadata, including things like description, available actions, etc...) Below is an example of how this is handled in Coinbase:
Screen Shot 2021-02-08 at 2 16 28 PM

If this is something we would like to include in this iteration, I am happy to design the view as soon as we can confirm what metadata we would have available.

  1. Once we have the ability to display fiat conversions in the wallet, it would benefit users to have that information displayed along with each token, and so that we can show users' aggregate balances using a fiat conversion of their choice. Until then, there is no easy way for us to show the "cumulative value" or performance of a user's assets, and we cannot include fungible tokens in the user's NEAR balance. This is a bit clunky, and it would be nice to establish a baseline fiat conversion in order to be able to compare and aggregate asset value more elegantly in the wallet.
heycorwin commented 3 years ago

There are certainly some things here up for debate as far as what fits into the scope of this work, so we can certainly adjust or extract work as necessary depending on scope.

stefanopepe commented 3 years ago

I agree with the proposed solution.

I have minor concerns/queries on the following items:

heycorwin commented 3 years ago

Re: "Approved Apps", this list is a list of applications that you have already authenticated and used, and that currently have permission to interface with your wallet account. While related, I view this more as an authentication setting than identical to the issue of creating an opportunity for new users to find apps that they might like to use. In the case of this issue, I'm simply proposing we relocate the list of authenticated applications to the profile, where similar authentication settings can be found.

kcole16 commented 3 years ago

I like the proposed solution!

Re: #7 (authorized apps to /profile), we do have the page /authorized-apps, and we could consider directing users to this page more explicitly rather than adding this section to the /profile page.

Re: token display, we should consider showing the actual token account name as well, as we have the benefit over Ethereum of this name being human readable and potentially useful for verification (though there are some phishing vectors here).

Timing is my only other concern. If we feel confident we can implement the above in ~2-3 weeks, let's go for it. Otherwise, we should consider how best to implement it iteratively to meet the goal of launching token support by March 1 or so.

heycorwin commented 3 years ago

Re: token display, we should consider showing the actual token account name as well, as we have the benefit over Ethereum of this name being human readable and potentially useful for verification (though there are some phishing vectors here).

Happy to work this in, although I'm curious to hear articulated what we think the biggest benefit is to displaying outside of a "details" or "metadata" view?

Timing is my only other concern. If we feel confident we can implement the above in ~2-3 weeks, let's go for it. Otherwise, we should consider how best to implement it iteratively to meet the goal of launching token support by March 1 or so.

Yes absolutely, I'll have @vgrichina and @Patrick1904 weigh in here. If we think this is out of scope for the proposed timeline, please suggest which elements we think would be best to omit.

Patrick1904 commented 3 years ago

Timing is my only other concern. If we feel confident we can implement the above in ~2-3 weeks, let's go for it. Otherwise, we should consider how best to implement it iteratively to meet the goal of launching token support by March 1 or so.

IMO we should go with the absolute lowest lift solution this first implementation considering no work has been done so far on this and since we need it in by March 1. It's very soon. We could ship a more fancy v2 a week or two after initial version.

heycorwin commented 3 years ago

@Patrick1904 yep I'm on board with this approach. Please specify what you believe should be trimmed / is out of scope.

Patrick1904 commented 3 years ago
Screen Shot 2021-02-08 at 2 06 37 PM

@corwinharrell how about for V0 we implement the portfolio/wallet screen and the activity screen? Also, should we include activity on the wallet page vs stand alone, as it's the 'default' page? Essentially put activity below the secondary assets. Personally I look at the activity feed quite a lot and would prefer to not have to navigate to a separate page for it.

vgrichina commented 3 years ago

sorry that this wasn't very clear in https://gist.github.com/vgrichina/f39623e3cc80b3a4d36fc8eb81a49374

but basically we don't really have a master list of tokens what we have is a list of contracts that user interacted with (and potentially that have been sent to user)

the only part in the data that we for sure can trust is contract account ID

everything besides contract account ID is returned by the contract and so can potentially be spoofed by the contract (user has to trust specific contract to use it). There can be balances of the same type of token in multiple different contracts as well.

so the structure is more like this:

basically we have multiple contract each supposedly having multiple assets in it we cannot easily have a flat list of assets given what is actually on chain

even if we hardcode for fungible tokens only assuming that we only have one balance per contract – we have to surface actual contract IDs, as basically anybody can make their contract return BNNA as token symbol

vgrichina commented 3 years ago

Token whitelist would solve the security aspect of trusting token symbol. However I think we shouldn't build a system that is only scalable for whitelisted tokens, as it seriously limits adoption of any new tokens. Like basically social tokens won't stand a chance in such system.

This also doesn't scale for our existing apps like Berry Club which have multiple balances per contract.

vgrichina commented 3 years ago

Small refactor to how activity items are displayed in order to make them easier to consume and scan.

We also need to look into how to display activity with fungible tokens, as right now these would basically be pretty opaque function calls.

vgrichina commented 3 years ago

what we have is a list of contracts that user interacted with (and potentially that have been sent to user)

WIP for it on backend is here https://github.com/near/near-contract-helper/pull/322/files

heycorwin commented 3 years ago

@vgrichina thanks for the clarification. I was definitely working off an incorrect set of assumptions based on your first example. Here's a thought and a question. It's clear that since the way we access fungible token balances isn't via a whitelist, we can't simply display tokens as they would appear say, on an exchange. There is benefit though to adhering to a similar model when it comes to displaying them in the context of the wallet, just because it's what people are used to seeing, and it's in our best interest not to introduce too many new concepts and mental models. I'm curious if there's a middle ground to be struck here. Would it be possible to display tokens on the wallet screen as a list, but then make sure to denote the contract to which they belong? Here's an example:

Frame 321

Since multiple contracts can contain the same token, I'm wondering if we can work in this direction, and then provide a breakdown of how your tokens may be distributed. To explain further, here's an example:

The reason I favor this approach is that it bubbles up to a "total" on the top level screen, where a user is most likely less concerned with contract details, and more concerned with token balances and the cumulative value of their portfolio. Then if they need to see details, it makes sense for them to drill down to see exactly which contracts of theirs hold that asset.

My biggest assumption in all of this is that we can aggregate token balances from multiple contracts in order to display them on the summary view. This is different from a "whitelist", since the token balances are extracted from existing contracts, and are only aggregated if their data matches.

I could be totally off base here if my understanding is still flawed, so just let me know if there are any holes in this thinking. Will defer to you to determine feasibility of each component.

stefanopepe commented 3 years ago

I support @corwinharrell 's approach, but we still need to solve the "false BNNA problem", where the user is using a contract claiming to host BNNAs, but they are fake.

heycorwin commented 3 years ago

Design Complete: https://www.figma.com/file/MOMOnm89esHGC4PMKDBEDw/Fungible-Token-Balances?node-id=0%3A1

Patrick1904 commented 3 years ago

Note on what this project includes:

  1. New total balance and actions (send/receive) on dashboard
  2. New Fungible tokens section on dashboard
  3. Re-designed 'activity' section on dashboard
  4. Re-designed navigation
  5. Re-designed 'authorized apps' section - moved from dashboard to profile
heycorwin commented 3 years ago

@Patrick1904 @marcinbodnar please be vocal and ruthless about scoping this stuff down. Several of these are not essential to displaying fungible token balances, but rather are downstream changes which we can afford to make after the current sprint. They improve the experience quite a bit, but shouldn't prevent us from meeting the March 1 deadline. Only commit to what you feel comfortable taking on.

Patrick1904 commented 3 years ago

Closing as this has been released in https://github.com/near/near-wallet/pull/1551