feat: Fidelity Bonds #307
feat: Fidelity Bonds #307
Conversation
theborakompanioni
added
enhancement
theborakompanioni
added
the
blocked
theborakompanioni
force-pushed
the
fidelity-bonds-simple
branch
2 times, most recently
from
af67373 to
acb041e
Compare
theborakompanioni
force-pushed
the
fidelity-bonds-simple
branch
from
665c519 to
ba1c7e3
Compare
Co-authored-by: Gigi <109058+dergigi@users.noreply.github.com>
Co-authored-by: Gigi <109058+dergigi@users.noreply.github.com>
theborakompanioni
force-pushed
the
fidelity-bonds-simple
branch
from
9c7ce1c to
ae68dc4
Compare
|
Ok let's merge this now and work do any updates in separate PRs. Unless you're still reviewing @dergigi? 🤔 Imo I'd be nice to then:
|
theborakompanioni
force-pushed
the
fidelity-bonds-simple
branch
from
e8f725b to
833dd64
Compare
There was a problem hiding this comment.
Nice! Good job on all the tests, and on getting the first iteration through the door! 👏 Also: thank you @dnlggr for all the work on this in terms of sparring and review.
Tested locally on regtest, similarly to @theborakompanioni's demo in Call 17.
I only have nits, thus this can be merged I think. Looks good to me ✅
| calculatedTotalBalanceInSats: accountTotalCalculated, | ||
| calculatedFrozenOrLockedBalanceInSats: accountFrozenOrLockedCalculated, | ||
| calculatedAvailableBalanceInSats: accountAvailableCalculated, | ||
| accountIndex: parseInt(account, 10), |
There was a problem hiding this comment.
NIT: Any reason for the explicit 10 in all the parseInt calls? 2nd param could be removed, 10 is default afaik.
There was a problem hiding this comment.
Over the years, I have made a habit of always specifying the radix to get reliable results.
Be careful—this does not default to 10!
See: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/parseInt
JavaScript is strange.
Edit: In this case, it would probably be fine, as we should not expect "malformed" inputs from JM.. nonetheless.. I guess I'll sleep better with the radix included. Is that okay for you?
There was a problem hiding this comment.
JavaScript is strange.
Ain't that the truth! 😅
| // This is useful in simple mode - when it should be prevented that users | ||
| // lock up their coins for an awful amount of time by accident. | ||
| // In "advanced" mode, this can be dropped or increased substantially. | ||
| export const DEFAULT_MAX_TIMELOCK_YEARS = 10 |
There was a problem hiding this comment.
10 years is quite a lot 😅 -- that being said, I don't know what a sane maximum should be. 10 years is probably good!
There was a problem hiding this comment.
Yeah.. as far as I can see most fidelity bonds are not beyond the year 2026.. should we decrease to a maximum of 5 years and wait for user feedback?
Agree, also with next steps 👍 |
ghost
merged commit 
ghost
mentioned this pull request
First draft of Fidelity Bonds.
It currently looks nothing like the great designs by @editwentyone, but some building blocks are be present and hopefully it can be easily adjusted step by step from now on.
This PR has all previously implemented coin control features removed (moved to branch
fidelity-bonds-coin-control-backupin case they are needed later)This means that all available funds in a jar will be used to create the fidelity bond.
Thanks @dnlggr for your valuable feedback – it should now be much more easily reviewable.
Maybe some coin control features from #241 can be used and make it more configurable for users.
Some questions are still unanswered or need to be addressed:
minimum_makersis used – which is not optimalSendscreen?Internationalization (i18n) is not addressed as the UI will be reworked in subsequent PRs and it would be obsolete very quickly.