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

frontend simpler coinbalance #2714

Closed

Conversation

thisconnect
Copy link
Collaborator

Coin balance needs a list of coinCodes and some way to display the
coin name.

Before this change it first constructed a map that contains accounts
per coin, as the name of the coin is only included in IAccount
interface.

Using a dedicated utils function to derive the coin name from coin
codes simplifies CoinBalance component.

Also cleanup and type keys of various maps in 2nd commit

Before the key was just of type string, but in these cases
we have proper types.

@strmci
Copy link
Collaborator

strmci commented May 15, 2024

tested, nice

I would name testnet coins so these names are consistent with the names in backend.go
tbtc: Bitcoin Testnet
tltc: Litecoin Testnet
goeth: Ethereum Goerli
sepeth: Ethereum Sepolia

image

It will be nice to have it sorted as Bitcoin, Litecoin, and Ethereum...

Are all the other coins in the getCoinName needed? Can't we just return coinCode?

@thisconnect thisconnect force-pushed the frontend-simpler-coinbalance branch from 9d3d59f to b2d2c47 Compare May 15, 2024 08:54
@thisconnect
Copy link
Collaborator Author

@strmci I address your feedback, it should use correct names i.e. "Bitcoin Testnet" for tbtc etc.

I think the other tokens would be needed but the backend doesn't return those for total coin balance I think. So I removed those for now. But probably the backend should return all coins and tokens for the total coin balance.

also it sorts now correctly... but it sorts in the frontend. Other places we keep the sort as it is coming from backend (accounts), but here we don't use accounts anymore (and we shouldn't imo).

If that all is good as is, I'd like to also get feedback from @Beerosagos

@thisconnect
Copy link
Collaborator Author

actully on master it shows tokens.. need to figure out how I broke it...

@thisconnect thisconnect force-pushed the frontend-simpler-coinbalance branch from b2d2c47 to a1fa9a4 Compare May 15, 2024 09:53
Copy link
Collaborator

@strmci strmci left a comment

Choose a reason for hiding this comment

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

But probably the backend should return all coins and tokens for the total coin balance.

I think it should return coins only from active accounts as it currently does.

tested on both testnet and mainnet, LGTM

@thisconnect
Copy link
Collaborator Author

yes is does, I somehow thought backend doesn't show tokens totals.

'eth-erc20-wbtc',
'eth-erc20-paxg',
'eth-erc20-dai0x6b17',
] as accountApi.CoinCode[];
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is not correct as type CoinCode does not include the tokens. Type casting is not ideal here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added another commit that properly types CoinOrTokenCode ... but now it's huge.
But I could remove this type cast here. 😅

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

IMO we could also drop the 3rd crazy typescript commit and I open a separate PR, let me know

opened a new PR #2717 so that this can be merged.

Coin balance needs a list of coinCodes and some way to display the
coin name.

Before this change it first constructed a map that contains accounts
per coin, as the name of the coin is only included in IAccount
interface.

Using a dedicated utils function to derive the coin name from coin
codes simplifies CoinBalance component.
Before the key was just of type string, but in these cases
we have proper types.
@thisconnect thisconnect force-pushed the frontend-simpler-coinbalance branch from 9b4e582 to a71be33 Compare May 16, 2024 10:43
@thisconnect
Copy link
Collaborator Author

rebased

@thisconnect
Copy link
Collaborator Author

@strmci ack to merge? 😇

@strmci
Copy link
Collaborator

strmci commented May 16, 2024

@strmci ack to merge? 😇

tACK

@Beerosagos
Copy link
Collaborator

The code looks good to me, but I feel like we should rely on the backend sorting if possible to avoid duplicates.
Also what about we add the coin name inside the coin total balance response?

CCing @benma as he also had an opinion about leaving sorting in the backend, iirc

@strmci
Copy link
Collaborator

strmci commented May 20, 2024

Also what about we add the coin name inside the coin total balance response?

I also checked that option. Do you mean to add it to the FormattedAmount type, or?

@Beerosagos
Copy link
Collaborator

Also what about we add the coin name inside the coin total balance response?

I also checked that option. Do you mean to add it to the FormattedAmount type, or?

I was thinking about enclosing the formattedAmount inside a CoinFormattedAmount object, together with the coin name, and returning it inside the map produced by this function. This way we don't propagate the change in every place where we use the FormattedAmount.
But your idea could be good too, I'm not sure about which one is the better approach, didn't think much about it yet

@thisconnect
Copy link
Collaborator Author

I think we should re-consider doing both (visual-coin-order-sort & coin-name) in the frontend.

Coin name lookup by coinCode is very simple, see this PR. Not sure the backend needs to know about human readable coinName at all.

Sorting in the frontend seems to me is much easier than in the backend. Is the sorting in the backend used for anything else that showing this order in the frontend?

Note: Some api's return items in an array [item, item, ...] in order. Other API's return an objects / map {}. Objects are not iterable and don't have any information about how the items are ordered. We could add an index property to each item in the object of course as a workaround.. but then why do this in the backend?

@thisconnect
Copy link
Collaborator Author

I think there is some good typescript improvements in this PR. I'll try to make a separate smaller one.

Backend should keep controlling sort and provide coin names, this should come from backends for example if we ever had a different UI.

@thisconnect
Copy link
Collaborator Author

closing in favor of only typscript improvements #2733

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.

None yet

3 participants