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: Fidelity bonds in developer mode #221

Closed
wants to merge 22 commits into from

Conversation

theborakompanioni
Copy link
Collaborator

@theborakompanioni theborakompanioni commented Apr 15, 2022

Part of #31.

This PR contains a minimalist screen to create Fidelity Bonds in advanced mode.
It is behind a feature flag that is only activated for local development.

Fidelity Bonds for normal mode is a bit comprehensive and is done in follow-up PR.

Currently, a link from the CurrentWalletAdvanced view leads to a simple form that let's users choose the locktime and will subsequently display the corresponding address. As this is in advanced mode, a user can then manually fund this address by himself.

In the normal mode, the fidelity bond should be created automatically.

Early state. This is just a proof of concept. All feedback very welcome.
Keep in mind that it is still hidden behind a feature flag!

jamjamjam_fb_create_advanced.mp4

It would be great to have this merged for local development, even if it is not fully polished yet (e.g. some translation strings are missing).

@theborakompanioni theborakompanioni self-assigned this Apr 15, 2022
@theborakompanioni theborakompanioni added concept Wild idea, or too many details unknown yet WIP Work in Progress labels Apr 15, 2022
@theborakompanioni theborakompanioni changed the title feat: Fidelity bonds feat: Fidelity bonds in advanced mode Jun 1, 2022
@theborakompanioni theborakompanioni removed the WIP Work in Progress label Jun 1, 2022
@theborakompanioni theborakompanioni marked this pull request as ready for review June 1, 2022 12:51
@theborakompanioni theborakompanioni requested a review from a user June 1, 2022 12:51
@theborakompanioni theborakompanioni marked this pull request as draft June 3, 2022 13:00
@theborakompanioni
Copy link
Collaborator Author

Converted to draft. Will update "dark mode" and remove "Advanced Mode" toggle.

@theborakompanioni
Copy link
Collaborator Author

theborakompanioni commented Jun 6, 2022

Updated:

  • fixed dark mode
  • remove "advanced view": Simple (not implemented) and DevOnly view are now separated into two distinct components

Everything said before is still true:

  • Not polished – however, would be good to have it in yet
  • It is hidden behind a feature flag and only available in local development
  • Missing translation strings and other stuff will be done in follow-up PRs (e.g. feat: Fidelity Bonds #307)

Needs a review.

@theborakompanioni theborakompanioni marked this pull request as ready for review June 6, 2022 14:47
@theborakompanioni theborakompanioni changed the title feat: Fidelity bonds in advanced mode feat: Fidelity bonds in developer mode Jun 6, 2022
src/libs/JmWalletApi.ts Show resolved Hide resolved
src/components/CurrentWalletAdvanced.tsx Outdated Show resolved Hide resolved
src/components/CurrentWalletAdvanced.tsx Outdated Show resolved Hide resolved
Comment on lines +53 to +55
<Link className="unstyled" to={routes.fidelityBondsDevOnly}>
Switch to developer view.
</Link>
Copy link

Choose a reason for hiding this comment

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

Honestly I don't think we need that extra step. It's already only in "advanced" mode (which is called developer mode in the app already). Once the user is here I'd say we can let them do their stuff. No need for the extra hurdle imo. What do you think about it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think we need two distinct modes. As a developer, I want to have access to the actual timelock address, which does not necessarily need to be exposed to a user ever. Do you have a preferred way to do this or do you have different mechanism in mind?

Having a dedicated view for local development seemed to be the easiest solution to me.
e.g. Creating Fidelity Bonds in the past – a user should never be able to do that.. in development it is very useful, though.

return (
<rb.Row>
<rb.Col xs={6}>
<rb.Form.Group className="mb-4" controlId="locktimeYear">
Copy link

Choose a reason for hiding this comment

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

It's a shame that Firefox still doesn't support the "month" date picker. It's been tracked for over 9 years... Same for Safari.

Anyway what do you think of a dropdown here? At least for the months a 12 value dropdown is something users will know from most websites. The same applies to the year as well I guess. Most users won't pick a date several decades ahead (I guess) so the year will be accessible quickly in the dropdown.

Longer term, we should go with a nice date picker of course but this can be an improvement. Shame on you Firefox and Safari.

Choose a reason for hiding this comment

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

can we implement something like this?
image
Source: https://www.primefaces.org/primeng/calendar

Choose a reason for hiding this comment

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

Copy link

Choose a reason for hiding this comment

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

can we implement something like this?

Not out of the box unfortunately since Firefox and Safari don't support the default HTML "month-only" date pickers (see links above for more details).

We can (and should) certainly do something custom or use a library but that's definitely something for a separate PR.

src/components/FidelityBondDevOnly.tsx Outdated Show resolved Hide resolved
Comment on lines 20 to 23
const dateToLockdate = (date: Date): Api.Lockdate =>
`${date.getUTCFullYear()}-${date.getUTCMonth() >= 9 ? '' : '0'}${1 + date.getUTCMonth()}` as Api.Lockdate

const lockdateToTimestamp = (lockdate: Api.Lockdate): number => {
Copy link

Choose a reason for hiding this comment

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

These two would be ideal for some unit tests.

src/components/FidelityBondDevOnly.tsx Outdated Show resolved Hide resolved
src/components/FidelityBondDevOnly.tsx Outdated Show resolved Hide resolved
const [lockdateMonth, setLockdateMonth] = useState(currentMonth)

useEffect(() => {
const date = new Date(Date.UTC(lockdateYear, lockdateMonth - 1, 1, 0, 0, 0))
Copy link

Choose a reason for hiding this comment

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

Suggested change
const date = new Date(Date.UTC(lockdateYear, lockdateMonth - 1, 1, 0, 0, 0))
const date = new Date(Date.UTC(lockdateYear, lockdateMonth - 1, 1))

Copy link

Choose a reason for hiding this comment

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

This comes up quite often and not forgetting the - 1 part is critical. Should we move this to a function?

@ghost
Copy link

ghost commented Jun 6, 2022

What do you think of adding a "deposit"/"send"/"lock" button to the form so that the user doesn't have to go to the Send page? That way the fidelity bond feature would be self contained (Even though some API calls will have to be duplicated from the Send page. But that's fine imo.)

@editwentyone
Copy link

I think it was a rough first poc with the generation of the address. we should do all the magic behind the curtain. no Manuel sending, don't you think?

@ghost
Copy link

ghost commented Jun 6, 2022

I think it was a rough first poc with the generation of the address. we should do all the magic behind the curtain. no Manuel sending, don't you think?

Yes definitely. We can obviously also split that up into several PRs if you prefer that @theborakompanioni. Anything goes, it's behind a feature flag anyway :)

@theborakompanioni
Copy link
Collaborator Author

Hey guys, thank you very much for your feedback. I'll try to integrate it soon and will keep all things said here in mind while working on #307 today. Will ping you, once updates were made.

@theborakompanioni theborakompanioni force-pushed the fidelity-bonds branch 2 times, most recently from b4bedff to 784ae27 Compare June 7, 2022 17:06
@theborakompanioni
Copy link
Collaborator Author

Closing this in favor of #307 and try to address all feedback there.

@theborakompanioni theborakompanioni deleted the fidelity-bonds branch June 10, 2022 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
concept Wild idea, or too many details unknown yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants