feat: move fidelity bonds to earn screen #361
Conversation
ghost
added
enhancement
ghost
self-assigned this
|
Tested it a bit and must say: It rocks 🪨 🪨 🪨 🚀 Aside from the things you are already aware of or you have even mentioned above: What follows are random thoughts, in no particular order, I am sure you also had during development. Some of them have been also brought up in the previous Community Call. However, it is always beneficial to take note of them:
Small issues:
Other thoughts:
I guess the most important question is whether to keep coin control in the FB setup or complete externalize it. Great job by the way. Definitely better than anything I could come up with. Really nice 🧡 |
|
Thanks for the feedback @theborakompanioni, I've added a couple more things and updated the description accordingly. I'm marking this as ready for review now. It's still not perfect but I feel like anything else can be done in distinct improvement PRs. Let me know what you think! 🙏 |
ghost
marked this pull request as ready for review
There was a problem hiding this comment.
Great work @dnlggr! 👏
Code looks good to me in general, see NITs inline; only the one TODO is something that should be removed I believe.
I did my best to test this quite thoroughly, and out of the 6-or-so back-to-back test runs I think I managed to break it once. I created a fidelity bond and made some transactions and did some freeze/unfreeze operations, and suddenly the fidelity bond was gone. Not sure what happened, will try to reproduce it again tomorrow. I also managed to reproduce the Gateway timeout error around the same time, so maybe something just broke completely (something that is unrelated to this PR).
I'll give this a tACK now anyway, since we are running with scissors (and I wasn't able to reproduce whatever I did, and it's absolutely possible that nothing is wrong and I'm just stupid in the first place). So: LGTM ✅
I agree on the improvements listed, and here are some additional things that could be tracked for follow-up PRs:
- Add a back button for every step (currently there is no way to go back a step)
- Improve confusing iconography ("frozen" uses a lock; on the next step, "UTXOs that will be locked up" shows a lock again; same symbol for two things that are the opposite --> very confusing!)
UX wise the biggest improvements would be a button to batch-unfreeze all frozen UTXOs that were just frozen (or all UTXOs in the jar that were selected). Pre-selecting the largest cj-out or something like that would be cool too, that way you can just go "next next next". The batch-unfreeze is almost a must for 0.1.0 I'd say.
In any case, great job, this is already amazing for a first iteration! 💪 🚀
As mentioned inline, I will take care of the final copy in a separate PR.
| const { mixdepth } = utxo | ||
| res[mixdepth] = res[mixdepth] || [] | ||
|
|
||
| // todo: remove |
There was a problem hiding this comment.
Remove before merge? (I assume the below was a hack for testing cj-outs on regtest?)
Edit: I guess you left it in on purpose so we can test it. In that case: thanks! 😅 🙏
There was a problem hiding this comment.
Yep definitely needs to go before merging. Removed now. ✅
| <ConfirmModal | ||
| isShown={showConfirmInputsModal} | ||
| title={'Are you sure you want to lock your funds?'} | ||
| body={`Be aware that your funds can only be moved again when the fidelity bond expires on ${new Date( |
There was a problem hiding this comment.
Note to self: would be cool if we also show total duration in days/weeks/months/years. Here and also in the summary view.
| <div className="d-flex flex-column gap-4"> | ||
| <div className={styles.stepDescription}> | ||
| Fidelity bonds are a feature of JoinMarket which improves the resistance to sybil attacks, and therefore | ||
| improves the privacy of the system. This should be a better and easier to understand description and let the |
There was a problem hiding this comment.
😆 --- I will take care of the final copy, don't worry 😅
|
No major flaws or issues detected. Some minor stuff:
Most of the following is related to UI/UX - hence, my opinion should not be given too much weight.
NITs (just for reference):
|
Co-authored-by: Gigi <109058+dergigi@users.noreply.github.com>
| <div>locked</div> | ||
| </div> | ||
| )} | ||
| {!isLoading && !utxo.frozen && utxo.label === 'cj-out' && ( |
There was a problem hiding this comment.
Just noticed that the classification (new, deposit, cj-out, etc.) are not provided in the data from /utxo, but only in /display, where they are not on "utxo", but on "address" level..
Nonetheless, I think displaying the label here (even when it cannot be set by Jam yet), is a good idea.
This is an object in entries of the display response:
{
"0": {
"hd_path": "m/84'/1'/1'/1/0",
"address": "bcrt1qeme6gzjlj5s95jfe8hwauagy8y45teh4rej8jw",
"status": "cj-out",
"label": "",
"extradata": "",
"amount": "149.99994620",
"available_balance": "149.99994620"
}
}Compare with the utxo data structure (no status here):
{
"0": {
"address": "bcrt1qeme6gzjlj5s95jfe8hwauagy8y45teh4rej8jw",
"path": "m/84'/1'/1'/1/0",
"label": "",
"value": 14999994620,
"tries": 1,
"tries_remaining": 2,
"external": false,
"mixdepth": 1,
"confirmations": 7,
"frozen": false,
"utxo": "9dbd6db083b3fb23c936bd16dbc52efa64330a3145d4c8c9492136a00c8e4c80:1"
}
}There was a problem hiding this comment.
If I understand correctly, the API does not return the UTXO labels in the UTXO endpoint but only for the /display wallet endpoint on an address label and we need to manually match up UTXOs and addresses to find out which UTXO has which label? 🤔
Do you know what the difference between a label and a status is?
There was a problem hiding this comment.
If I understand correctly, the API does not return the UTXO labels in the UTXO endpoint but only for the
/displaywallet endpoint on an address label and we need to manually match up UTXOs and addresses to find out which UTXO has which label? thinking
Correct.
Do you know what the difference between a label and a status is?
A label is manually applied by the user (https://github.com/JoinMarket-Org/joinmarket-clientserver/blob/3140616d322c85e71591d8d4d46cbb4cea374002/docs/release-notes/release-notes-0.9.4.md#user-chosen-address-labelling). Whereas the status is a categorization applied by the backend (https://github.com/theborakompanioni/joinmarket-clientserver/blob/36bf36bd65f6365cb282a8abf3512e8e11df548a/jmclient/jmclient/wallet_utils.py#L493-L531).
There was a problem hiding this comment.
Do you know if there is there any reason for not having the status in the UTXO? 🤔
We'd need a way to check if the lockdate is in the past. Do you know what timezone the lockdate returned from JM is? I'm guessing UTC but can't find any docs. 😢 |
You are right of course. Removed the async/await keywords. Please have another look if you can. 🙏 |
It's a unix timestamp, so no timezone. Already present, just not well placed in |
ghost
mentioned this pull request
|
Thanks for the detailed feedback @dergigi and @theborakompanioni! 🙏 📋 I've tracked a whole bunch of follow up issues here. I've roughly split them up between v0.0.9 (issues that improve the current flow) and v0.1.0 (issues that are either only nice to haves or extend the current flow) although I do think that we can do an intermediate release and move some of the "not so high prio but still kinda important" issues to an intermediate v0.9.1 before we get going on the big v0.1.0. |
Resolves #339.
Moves fidelity bonds to the earn screen, updates the UI, and adds UTXO selection. Consider this the base version of the
feature that we can build upon in separate PRs.
👇 A fair chunk of the new lines of code are tests and CSS, don't worry too much about that number. 😄
📸 How does it look?
Screen.Recording.2022-07-05.at.15.13.29.mov
💡 Possible Improvements
I'll track these points as separate issues if you guys agree on them.
Things we MUST do before releasing
translation.jsonfile. At the moment everything is either dummy text or copied from Figma or elsewhere without much thought.Things we SHOULD consider doing before releasing if we have the time
CreateFidelityBondUI component and redo some of the logic to be more concise and less all-over-the-place.cj-outUTXOS (e.g. by making the final button red similarly to how we do it when sending a "normal" transaction on the Send screen.)Things we MIGHT consider as improvements for a future releases
Let me know what else you would like to see.