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(Jam): add success message #599

Merged
merged 8 commits into from
Feb 8, 2023
Merged

Conversation

theborakompanioni
Copy link
Collaborator

@theborakompanioni theborakompanioni commented Jan 23, 2023

Resolves #489.

This PR adds a success message after a scheduler run completes.

Before this PR, the page reloaded greeting the user with a message ala "The scheduler requires your UTXOs to have 5 or more confirmations", which is confusing to say the least.
After this PR, the user will see a screen indicating success with a short summary, i.e. "Successfully completed n scheduled transactions".

Please note that due to polling the scheduler data, this might miss the state change of the last scheduled item. To mitigate this situation, it is checked whether only frozen UTXOs remain in the wallet, which should always be the case on mainnet. However, in "dev mode" (where – on purpose – not all jars a swept in order to decrease the time the scheduler needs to run; an on top – the funds are sent to Jar A of the same wallet) this might still occur.

jam/src/components/Jam.tsx

Lines 249 to 260 in 8a0be65

const lastEntryState = lastKnownSchedule[lastKnownSchedule.length - 1][6]
const lastEntrySuccess = isInMempoolOrSuccess(lastEntryState)
// Workaround to prevent race conditions: Since the schedule info is polled,
// it'll be possible that the latest known state still has the success flag
// of the last entry set to `0`, although the schedule was completed successfully.
// In this case, additionally check that every remaining UTXO is frozen
// (indicating the opteration was successfully completed).
// Hint: In dev mode, this will only work if you send coins to an external wallet.
const allUtxosFrozen = walletInfo.data.utxos.utxos.every((it) => it.frozen)
setIsShowSuccessMessage(firstEntriesSuccess && (lastEntrySuccess || allUtxosFrozen))

If you have an idea on how to upgrade this from a workaround to a fully reliable fix, please say so!

As always, any feedback is greatly appreciated. As well as any improvement of wording/phrases/etc.!

How to test

Start the scheduler while mining some blocks. Wait for the scheduler run to be completed.
Since polling is done every 10 seconds, and mining in the regtest environment is also done every 10 seconds, the situation described above might apply and you won't see the success message. Either increase the time between regtest blocks by running watch -n 60 ./docker/regtest/mine-block.sh (blocks every 60 secs) instead of npm run regtest:mine (blocks every 10 secs), OR send the funds to an external test wallet (e.g. to one of the regtest makers, http://localhost:29080). As mentioned, this should only be an issue on regtest, not in production.

📸 Screenshots

@theborakompanioni theborakompanioni added enhancement New feature or request UI/UX Issue related to cosmetics, design, or user experience labels Jan 23, 2023
@theborakompanioni theborakompanioni self-assigned this Jan 23, 2023
@theborakompanioni theborakompanioni changed the title feat(Kam): add success message feat(Jam): add success message Jan 23, 2023
@theborakompanioni theborakompanioni force-pushed the feat/scheduler-success-screen branch 2 times, most recently from ae9dba7 to 6d096e8 Compare January 24, 2023 13:27
@theborakompanioni theborakompanioni force-pushed the feat/scheduler-success-screen branch from 6d096e8 to 8a0be65 Compare January 25, 2023 10:38
@theborakompanioni theborakompanioni marked this pull request as ready for review January 25, 2023 10:46
@dergigi
Copy link
Contributor

dergigi commented Jan 30, 2023

Awesome! That's very much needed.

Since polling is done every 10 seconds, and mining in the regtest environment is also done every 10 seconds,

What about using different intervals as defaults, possibly primes, so that the chance of this race condition to occur is minimized? 11 and 13 would work, but any two primes should work.

@dergigi
Copy link
Contributor

dergigi commented Jan 30, 2023

On another note: it's difficult to review the changeset in Jam.* because of the switch from .jsx to .tsx - now there's no diff to look at, as it's a complete rewrite.

Not a huge issue, but doing this kind of refactoring in a separate PR would be better.

@dergigi
Copy link
Contributor

dergigi commented Feb 8, 2023

Any thoughts on the "prime number" hack as a fix for the issue you raised, @theborakompanioni?

What about using different intervals as defaults, possibly primes, so that the chance of this race condition to occur is minimized? 11 and 13 would work, but any two primes should work.

Copy link
Contributor

@dergigi dergigi left a comment

Choose a reason for hiding this comment

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

I've changed the values to primes (11 and 13) so the chance of the race-condition you described happening will be reduced. I've tested it a bunch of times and it always worked for me on regtest (and I don't think these changes will break anything - famous last words, ha!). I'll merge this now and will prepare a release after that. Feel free to revert my changes if you think they could be problematic.

Great job on this, it's way prettier and intuitive now. 💖

tACK, LGTM ✅

txcountparams: [1, 0],
timelambda: 0.025, // 0.025 minutes := 1.5 seconds
stage1_timelambda_increase: 1.0,
liquiditywait: 10,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
liquiditywait: 10,
liquiditywait: 13,

@dergigi dergigi merged commit 531adb6 into master Feb 8, 2023
@dergigi dergigi deleted the feat/scheduler-success-screen branch February 8, 2023 12:44
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.

Scheduler: Success Screen
2 participants