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(orderbook): show fidelity bond value and locktime #766

Merged
merged 5 commits into from
May 28, 2024

Conversation

theborakompanioni
Copy link
Collaborator

@theborakompanioni theborakompanioni commented May 28, 2024

Resolves #754.

Displays amount and expiration date of fidelity bonds in the orderbook overlay.

How to test

Create a fidelity bond for one of the dev makers (e.g. on localhost:29080 - you need to stop the maker, create a fidelity bond, and restart the maker again). Then, reload the orderbook on the main dev instance and verify that the correct Fidelity Bond information is displayed.

📸

@theborakompanioni theborakompanioni added enhancement New feature or request UI/UX Issue related to cosmetics, design, or user experience labels May 28, 2024
@theborakompanioni theborakompanioni self-assigned this May 28, 2024
@theborakompanioni theborakompanioni force-pushed the feat/fb-value-and-locktime branch from 06256a8 to da316fd Compare May 28, 2024 08:46
@theborakompanioni theborakompanioni marked this pull request as ready for review May 28, 2024 08:47
Copy link
Contributor

@0xSaksham 0xSaksham left a comment

Choose a reason for hiding this comment

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

Altogether the changes look good. Definitely able to merge.

src/components/App.tsx Show resolved Hide resolved
@@ -102,6 +103,10 @@ interface OrderTableEntry {
bondValue: {
value: number
displayValue: string // example: "0" (no fb) or "114557102085.28133"
locktime?: number
Copy link
Contributor

Choose a reason for hiding this comment

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

This part looks fine! Should amount also be explicitly converted to string?

Copy link
Collaborator Author

@theborakompanioni theborakompanioni May 28, 2024

Choose a reason for hiding this comment

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

Hmm.. I think I know what you mean.. amount is passed as String to the Balance component. I think I'd rather want to have the type info in the object rather than storing it as plain string here. Okay for you?

Copy link
Contributor

@barrytra barrytra left a comment

Choose a reason for hiding this comment

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

It is somehow not working for me.
I created a fidelity bond and then clicked start earning(which is start maker i believe). But for me it show bond value 0.
Screenshot from 2024-05-28 21-08-33
Screenshot from 2024-05-28 21-08-18

I am not sure if I am doing it the right way. please help!!
Though code part looks completely fine to me.

@theborakompanioni
Copy link
Collaborator Author

theborakompanioni commented May 28, 2024

I am not sure if I am doing it the right way. please help!! Though code part looks completely fine to me.

It might take some time before the bond value appears - it worked for me after refreshing a couple of seconds later. Can you try it again? 🙏

Edit: There is a message "It might take some time for your offer and your bond to appear in your local orderbook." but it is not displayed, when your own offer is already included in the orderbook. Maybe it should be shown till offer and bond (if the user holds a bond) are displayed?

@barrytra
Copy link
Contributor

barrytra commented May 28, 2024

It might take some time before the bond value appears - it worked for me after refreshing a couple of seconds later. Can you try it again? 🙏

It got displayed but its value is 0.

@theborakompanioni
Copy link
Collaborator Author

It might take some time before the bond value appears - it worked for me after refreshing a couple of seconds later. Can you try it again? 🙏

It got displayed but its value is 0.

Have you waited a couple of seconds and refreshed once or twice?

@barrytra
Copy link
Contributor

are you asking me to wait after creating FB but before start earning?

@theborakompanioni
Copy link
Collaborator Author

theborakompanioni commented May 28, 2024

are you asking me to wait after creating FB but before start earning?

No, wait a little bit after you started the maker and the offers appears in the orderbook while the bond value is zero. Refresh the orderbook from time to time. The bond value should appear a bit delayed (your orderbook watcher actually contacts every maker–in this case "yourself"–in order to get the bond info).

Hint: You can use the Refresh button on the orderbook overlay.

@barrytra
Copy link
Contributor

ok i'll try that..
What i understand is that value initially will be 0 and then it will increase.. right?

@theborakompanioni
Copy link
Collaborator Author

ok i'll try that..

Thanks 🙏 Please report back.

What i understand is that value initially will be 0 and then it will increase.. right?

Zero means there is no bond (or not yet loaded). This is actually a good point.. do you think it would be better to show something else? e.g. or a "no bond" label?

@barrytra
Copy link
Contributor

I refreshed it many times but still its value is still 0.
Don't know if i am doing something wrong?

Zero means there is no bond (or not yet loaded). This is actually a good point.. do you think it would be better to show something else? e.g. or a "no bond" label?

we can do that but i think at the same time it can be confusing for the user as well. Maybe we can add some more info for user when value is 0(no bond) when he hovers over it.

@barrytra
Copy link
Contributor

Okay this works fine now.
There is still one issue left:

  • This doesn't look much pleasing in light mode, that needs to be fixed.
    Screenshot from 2024-05-28 23-09-32
    We can probably fix it here or in some other PR

@theborakompanioni
Copy link
Collaborator Author

Okay this works fine now.

Glad it worked out. Should have mentioned that the FB needs some confirmations 🤦 Sorry 😬

There is still one issue left:

* This doesn't look much pleasing in light mode, that needs to be fixed.
  We can probably fix it here or in some other PR

You are right. Balances in "light mode" on dark backgrounds (e.g. tooltips) are a problem. Needs to be addressed - I'd say it is okay to do in a follow-up PR. Okay for you?

Copy link
Contributor

@barrytra barrytra left a comment

Choose a reason for hiding this comment

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

yes we can do that in follow-up PR.

@theborakompanioni
Copy link
Collaborator Author

yes we can do that in follow-up PR.

Agreed. Tracked in #770. Thanks @barrytra! 🙏

@theborakompanioni theborakompanioni merged commit 3bcbdee into master May 28, 2024
3 checks passed
@theborakompanioni theborakompanioni deleted the feat/fb-value-and-locktime branch May 28, 2024 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request UI/UX Issue related to cosmetics, design, or user experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat(orderbook): show fidelity bond value and locktime
3 participants