-
Notifications
You must be signed in to change notification settings - Fork 11
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: add subintent polling #289
Conversation
00bf7bb
to
aa013d8
Compare
@@ -28,18 +28,20 @@ export type RadixButtonMode = keyof typeof RadixButtonMode | |||
export type PersonaData = { field: string; value: string } | |||
|
|||
export const RequestStatus = { | |||
fail: 'fail', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
either we use past tense of stick with present
failed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just moved it up in this const, we do not use RequestStatus.fail
everywhere but pure string as well so I'm hesitant to changing this to failed
.map(({ subintent_status }) => subintent_status), | ||
new Set<SubintentStatus>(['CommittedSuccess']), | ||
backoff, | ||
) as ResultAsync<SubintentStatus, SdkError>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make poll generic and add SubintentStatus
to the output signature
) | ||
|
||
const preauthorizationPollingLoop = async () => { | ||
await requestItemModule.getLookedUp().andThen((lookedUpItems) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not clear what lookedUp
is. Can we change this to getUnconfirmedTransactions
or getUnresolvedTransactions
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should align with request item status so what do you say for getPendingCommit
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getPendingCommit is great
) | ||
activePolling.set(item.interactionId, polling) | ||
polling.result.andTee((result) => { | ||
requestItemModule.updateStatus({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not awaiting the updateStatus
promise to resolve. Is this intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if the polling result is an error? Shouldn't that update the status of the request item?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not awaiting the updateStatus promise to resolve. Is this intentional?
I changed it however polling.result
is not awaited so it doesn't really matter if updateStatus
is.
what if the polling result is an error? Shouldn't that update the status of the request item?
I don't think it should. What's the reason of the failure? I changed it so if polling failed it starts again 🤡
packages/dapp-toolkit/src/modules/wallet-request/pre-authorization-request/subintent-builder.ts
Outdated
Show resolved
Hide resolved
🎉 This PR is included in version 2.2.0-dev.14 🎉 The release is available on: Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 2.2.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
No description provided.