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

Portfolio cards and Balance break down #358

Merged
merged 10 commits into from
Jun 13, 2021

Conversation

rithvikvibhu
Copy link
Collaborator

@rithvikvibhu rithvikvibhu commented Jun 2, 2021

Supersedes and closes #329.

Balance

This replaces the single "Locked" balance with 3 numbers, shown only when applicable:

  • Locked in live auctions = bids in BIDDING state auctions
  • Locked in reveal = bids in REVEAL state auctions (includes blind if not revealed yet)
  • Locked in finished auctions = bids that can be redeemed or registered

There's also a USD estimate based on coingecko's public API.

image

Cards

Below the balance section, a row of cards show actionable info that may or may not be time-constrained:

  • Reveal - Bids not yet revealed, along with the earliest-reveal-period-ending time
  • Renew - Names expiring in the next 2 months
  • Redeem - Redeeming lost bids and recovering HNS
  • Register - Registering won names and recovering the difference
  • Transfers in progress and ready to finalize

Just like locked balance blocks, irrelevant cards are hidden.

Since the information needs to be fetched from different places (and frequently! as it is on the main page), imo it made sense to make one pass over all bids and collect everything instead of calling createRegisterAll, createRedeemAll, etc. which will go through the same loops multiple times, separately.

image

Test

Either simply try out random actions (bids, reveals, transfers, etc.) or use this for reference (regest) - after starting at block 200, at block 228, all states are covered (except transfers).
image
Don't have a script to place these bids yet, will try to write one based on https://gist.github.com/pinheadmz/49e3fac7d797a99c3a78fb3ca0ddc012.

TODO

  • Add code to check "expired" status
  • Function for Finalize All
  • Function for Renew All - clicking on these buttons currently open the Your Bids page
  • Remove console.log() statements before merging

Copy link

@RobertLowe RobertLowe left a comment

Choose a reason for hiding this comment

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

Also, the 80 + ## + ## breakdown is nice, but also show the = total lockup as well as it currently does.

Design note, not sure we need the colors, but no strong preferences here. Reveal buttons style is not consistent with other buttons in the app

👍

app/pages/Account/functions.js Outdated Show resolved Hide resolved
try {
const response = await (
await fetch(
"https://api.coingecko.com/api/v3/simple/price?ids=handshake&vs_currencies=usd"

Choose a reason for hiding this comment

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

could this be a setting? usd/cad/eur etc

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hm yeah, can do that 👍

@rithvikvibhu
Copy link
Collaborator Author

Thanks for the feedback @RobertLowe!

Also, the 80 + ## + ## breakdown is nice, but also show the = total lockup as well as it currently does.

It takes up space, might have to reduce the font size for everything (it's scrollable, but nice to have the whole breakdown in one view.) Will see if it fits.

Design note, not sure we need the colors, but no strong preferences here. Reveal buttons style is not consistent with other buttons in the app

The colors indicate how serious the operation is (has a time limit, some pending action, just notices of something happening). We could lighten them a bit more.

I agree, black buttons are weird. Have any suggestions for them?

@rithvikvibhu rithvikvibhu mentioned this pull request Jun 6, 2021
@RobertLowe
Copy link

Thanks for keeping on top of this @rithvikvibhu

@rithvikvibhu @chikeichan I think all new features merging in should require tests, my face went a bit pale when I saw there were only 2 spec files in the entire application :S

@rithvikvibhu
Copy link
Collaborator Author

rithvikvibhu commented Jun 8, 2021

We do need tests, but like you said, the are only 2 of them (one of which is completely commented out).
But I think we need a proper format to write tests and should start with the basics/main features first, especially for large projects like these.
And only when there's a (sort of) standard around how to write them, can new features be required to have tests. Otherwise, we'll have a bunch of half baked tests that may need to be rewritten later (or never).

Maybe you could help start with those tests?

@RobertLowe
Copy link

@rithvikvibhu maybe in the future, I'm still learning hsd's tests https://github.com/handshake-org/hsd/blob/master/test I need a better foundation there first, but I'm starting to interact with regtest via my own project tests, maybe after that I can contribute more here

@@ -446,6 +447,12 @@ class WalletService {
await finalizeMany(wallet, names);
};

renewMany = async (names) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

not blocking for this PR - now that we have ledger, we should add ledgerProxy to all calls like this. We will need to separate renewMany to createRenewMany and sendRenewMany for ledgerProxy.

@rithvikvibhu rithvikvibhu marked this pull request as ready for review June 13, 2021 05:56
Copy link

@RobertLowe RobertLowe left a comment

Choose a reason for hiding this comment

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

Did the UI change?

Screen Shot 2021-06-13 at 3 59 03 AM

I liked the breakdown in
ui

@rithvikvibhu
Copy link
Collaborator Author

The main changes:
Before:

  • The first locked value was only for bids in BIDDING state
  • The second locked value was for bids in REVEAL state (including true bid values and not-yet-revealed blinds)

Now:

  • The first locked value is for bids in BIDDING as well as REVEAL (the true bid values that cannot be recovered for ~10 days)
  • The second locked value is for bids in REVEAL and not-yet-revealed (does not include true bid values, this is moved to first value)

This was done mainly because the "In reveal" label may be confusing as only a part of it would be immediately recoverable.
There are still 3 parts for Locked Balance, I think it just happens that (in your pic) all locked values fall in the first category so only 1 value is shown.
The only other UI change is removing the card buttons and making the whole card clickable.

@RobertLowe
Copy link

I don't see the Real 1 Bid / Renew / Redeem 1 bid, etc

Would this be because I just don't have any?

@rithvikvibhu
Copy link
Collaborator Author

Yup, only actionable cards (with an exception of transfers, which stay until finalized) are shown.

@chikeichan
Copy link
Contributor

@RobertLowe thank you for reviewing our PR! please join our discord if you are interested for more discussion on bob - https://discord.gg/qtpmPCgrCB

@chikeichan chikeichan merged commit 70580a3 into kyokan:master Jun 13, 2021
@rithvikvibhu rithvikvibhu deleted the portfolio-cards branch June 17, 2021 14:31
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.

3 participants