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: wait for postage stamp to be usable when bying it #352

Merged
merged 2 commits into from
Apr 29, 2022

Conversation

vojtechsimetka
Copy link
Contributor

@vojtechsimetka vojtechsimetka commented Apr 28, 2022

resolves #321

This is a very minimal implementation of the functionality. I would like to point out that the UX here is pretty bad as it does not tell the user what is happening (e.g. the stamp was bought waiting for it to be usable) and if for whatever reason the stamp is not usable until the DEFAULT_STAMP_USABLE_TIMEOUT = 120_000 is reached, it will fail with error message. My suggestion would be to merge this PR but have a design session with David how to improve the UX. On mainnet purchasing the stamp will take 30-90 seconds and then waiting for it to be usable also takes up to 90 seconds.

@bee-worker
Copy link
Collaborator

bee-worker commented Apr 28, 2022

🐝 PR preview in Swarm

Preview URL: https://bah5acgza3cqaahd4cdcan54ekeqq3nctmhvpqxphpxzz7pjixxrewj4njxyq.bzz.link
Swarm Hash: d8a0001c7c10c406f78451210db45361eaf85de77df39fbd28bde24b278d4df1
Commit Hash: 4887e57
Commit Message: refactor: simplified the waitUntilStampUsable function

Comment on lines 234 to 249
const timeoutPromise = (): Promise<never> =>
new Promise((_, reject) =>
setTimeout(() => {
timeoutReached = true
reject(new Error('Wait until stamp usable timeout has been reached'))
}, timeout),
)

const stampWaitPromise = async (): Promise<PostageBatch | undefined> => {
while (!timeoutReached) {
const stamp = await beeDebug.getPostageBatch(batchId)

if (stamp.usable) return stamp
await sleepMs(pollingFrequency)
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I usually do this with for loops, I think the upside is you don't need to race promises, no need for a separate setTimeout for the rejection, and the implementation is a single block that is simpler IMHO.

I am fine with this as well, just wondering if this has some pros I am unaware of 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey! Actually no benefits, mostly really just complications. I will refactor, thanks for the suggestion :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only advantage is that it spends exactly the timeout time unlike for loop which also adds the time spent on requests to check the stamp is usable . I don't see that as a problem, just wanted to point it out :)

@vojtechsimetka vojtechsimetka requested a review from Cafe137 April 29, 2022 05:07
@vojtechsimetka vojtechsimetka merged commit 1e2face into master Apr 29, 2022
@vojtechsimetka vojtechsimetka deleted the feat/wait-for-postage-stamp-to-be-usable branch April 29, 2022 11:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Buying postage stamp does not wait for it to be usable
3 participants