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

fix: error interactions regression #293

Merged
merged 3 commits into from
Dec 6, 2024

Conversation

dawidsowardx
Copy link
Contributor

failed interactions were never resolved in connect button

failed interactions were never resolved in connect button
@dawidsowardx dawidsowardx force-pushed the fix-error-wallet-response-regression branch from 26208f5 to 4254878 Compare December 5, 2024 20:21
Copy link

github-actions bot commented Dec 5, 2024

Phylum Report Link

),
)
)
.mapErr((error) => logger?.error({ method: 'addWalletResponses', error }))
Copy link
Collaborator

Choose a reason for hiding this comment

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

returns the logger object ILogObjMeta

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah saw that decided to not change as it's not a problem, this error is not consumed anywhere after that. I can make it return undefined

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or map to SdkError just for the sake of consistency?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's not consumed. Does not really matter

): ResultAsync<unknown, SdkError> =>
): ResultAsync<
WalletInteractionSuccessResponse,
SdkError | WalletInteractionFailureResponse
Copy link
Collaborator

@xstelea xstelea Dec 6, 2024

Choose a reason for hiding this comment

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

can we keep it as SdkError only?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No because it'd not be adequate. It can error with FailureResponse that's actually the whole point of this regression :)

isSupported: () => _isSupported,
destroy: () => {},
disconnect: () => {},
send: () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

why not use vitest fn for this purpose?

@dawidsowardx dawidsowardx merged commit 719c02d into develop Dec 6, 2024
11 checks passed
Copy link

github-actions bot commented Dec 6, 2024

🎉 This PR is included in version 2.2.0-dev.16 🎉

The release is available on:

Your semantic-release bot 📦🚀

@dawidsowardx dawidsowardx deleted the fix-error-wallet-response-regression branch December 6, 2024 18:14
Copy link

🎉 This PR is included in version 2.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants