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: Prevent creating multiple fidelity bonds with same expiry date #616

Closed
theborakompanioni opened this issue Apr 21, 2023 · 9 comments · Fixed by #741
Closed

feat: Prevent creating multiple fidelity bonds with same expiry date #616

theborakompanioni opened this issue Apr 21, 2023 · 9 comments · Fixed by #741
Assignees
Labels
enhancement New feature or request Fidelity Bonds Label for grouping FB issues good first issue Good for newcomers
Milestone

Comments

@theborakompanioni
Copy link
Collaborator

This request stems from a user suggestions. Thank you, btw 🙏

As the title says, as Jam is tailored towards non-expert users, address reuse should be prevented (as much as possible) when creating fidelity bonds . Since the address of the bond is shown beforehand, if someone really wants to do this - for whatever reason - it can still be done manually or by using the CLI. However, the UI should try to stop this from happening if possible.

Out of scope:
Since Jam does not know any historical, expired (and subsequently moved) fidelity bonds, recreating FB with the same expiry date afterwards cannot be prevented. However, should it be prevented to create multiple FBs in the first place?

@theborakompanioni theborakompanioni added enhancement New feature or request good first issue Good for newcomers labels Apr 21, 2023
@editwentyone editwentyone added this to JAM Oct 13, 2023
@theborakompanioni theborakompanioni added the Fidelity Bonds Label for grouping FB issues label Mar 12, 2024
@kristapsk
Copy link
Contributor

Since Jam does not know any historical, expired (and subsequently moved) fidelity bonds, recreating FB with the same expiry date afterwards cannot be prevented.

Actually it can. Jam could do request to Esplora (blockstream.info) and / or mempool.space APIs via Tor, then at least display warning to the user if address has been used before. But need to be sure to not reuse Tor circuits for multiple requests for privacy reasons and also handle situations when API service(s) are down.

https://github.com/Blockstream/esplora/blob/master/API.md#get-addressaddress

https://mempool.space/docs/api/rest#get-address

@theborakompanioni
Copy link
Collaborator Author

Hey @kristapsk. Appreciate any contribution and suggestion from you. I have a hard time wrapping my head around it, as Jam is still limited to the browser and would only be able to do this in special environments, e.g. if Tor browser is used. Otherwise we'd need a server component running alongside a Tor daemon (i.e. on the same machine as the backend). As you know, the possibility of a small server component comes up time and time again, but this adds yet another burdon on the user. I have always had hoped that any information Jam needs can come from JM itself (mempool info, transaction history, etc.). Let me know what you think.

@editwentyone
Copy link

that sounds promising for so many different problems and use cases. lets hope it can be done directly inside JM :) right @kristapsk right?! :D

@barrytra
Copy link
Contributor

hey @theborakompanioni , i am Bhavesh (SOB applicant). I have gone through the issue, and what i understand is that we need to prevent creating multiple fidelity bonds with same expiry date to avoid address reuse. I on my development setup and i think this is the issue..
Screenshot from 2024-04-13 21-54-26
I already created a bond which is to be expired in august. And when i create a new one. It suggests me that same month, whereas it should not.. right? please let me know if I am going in the right direction.
Also do we have to completely avoid the expiry date, or should we suggest user to avoid it?

@theborakompanioni
Copy link
Collaborator Author

I already created a bond which is to be expired in august. And when i create a new one. It suggests me that same month, whereas it should not.. right? please let me know if I am going in the right direction. Also do we have to completely avoid the expiry date, or should we suggest user to avoid it?

Hey @barrytra! Yes, you are completely right, you are going into the right direction.

I think - so this is a personal opinion - that we should not prevent the user from creating a Fidelity Bond with the same expiry date if that is what he wants to do - instead I think a big warning message with a button that say "I understand what I am doing - I really want to create another Fidelity Bond with the same expiry date" is okay.

However, there are some things to consider, e.g. in most cases, it does not make sense to create multiple FBs in the first place.
Currently, the option to create another FB is very prominent - we should keep that in mind too. Also, if someone wants to create another FB with a same expiry date, it does not really make any sense to create an additional one with a lower amount - I cannot think of any reason where that makes sense, other than wanting to lock coins - for which I am sure there are other tools available.

Finally, there is a ticket for a complete overhaul of the FB creation flow: #650
We should tackle that after this feature is implemented. Currently, the code regarding FBs is of rather poor quality and not very pretty.

@barrytra
Copy link
Contributor

cool I am working on the issue, and done with the logic part. Now I have to mess a bit with frontend to display the warning message. please let me know if this message "I understand what I am doing - I really want to create another Fidelity Bond with the same expiry date" is enough to display or we should add anything else.

Can you also assign me this issue, if this is how things work here. Although I can start a draft pull request as well.

Probably we should get this issue fixed by tomorrow :)

@theborakompanioni
Copy link
Collaborator Author

cool I am working on the issue, and done with the logic part. Now I have to mess a bit with frontend to display the warning message. please let me know if this message "I understand what I am doing - I really want to create another Fidelity Bond with the same expiry date" is enough to display or we should add anything else.

ping @editwentyone

Can you also assign me this issue, if this is how things work here. Although I can start a draft pull request as well.

Assigned. 🙌

Probably we should get this issue fixed by tomorrow :)

Hey, no pressure and no time schedule necessary. Let's discuss in the PR. Undraft it whenever you feel a review is due.

@barrytra
Copy link
Contributor

cool I am working on the issue, and done with the logic part. Now I have to mess a bit with frontend to display the warning message. please let me know if this message "I understand what I am doing - I really want to create another Fidelity Bond with the same expiry date" is enough to display or we should add anything else.

@editwentyone please have a look

@editwentyone
Copy link

you could make a [checkbox] + text: "I understand what I am doing - I really want to create another Fidelity Bond with the same expiry date" that needs to be checked before the "Next" Button is available for you.

this checkbox is only visible when we checked that the same month + year is selected

good work :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Fidelity Bonds Label for grouping FB issues good first issue Good for newcomers
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants