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 virtuals.io transactions in app browser #6383

Merged
merged 1 commit into from
Jan 7, 2025
Merged

Conversation

walmat
Copy link
Contributor

@walmat walmat commented Jan 6, 2025

Fixes APP-2247

What changed (plus any additional context for devs)

Strips non standard params extParams causing transactions to fail on virtuals.io

Screen recordings / screenshots

Screenshot 2025-01-06 at 4 22 07 PM

What to test

virtuals.io transactions

Copy link

linear bot commented Jan 6, 2025

@brunobar79
Copy link
Member

Launch in simulator or device for 8c9895b

Copy link
Member

@brunobar79 brunobar79 left a comment

Choose a reason for hiding this comment

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

I'm approving this so we can fix it asap but I don't like the fact that our TX parsing is so fragile that any dapp passing any non expected param would break.

I think we need to refactor this so we ignore everything except for the stuff we care.

Created https://linear.app/rainbow/issue/APP-2248/refactor-tx-parsing-from-dapps-so-it-doesnt-break

@walmat
Copy link
Contributor Author

walmat commented Jan 7, 2025

I'm approving this so we can fix it asap but I don't like the fact that our TX parsing is so fragile that any dapp passing any non expected param would break.

I think we need to refactor this so we ignore everything except for the stuff we care.

Created https://linear.app/rainbow/issue/APP-2248/refactor-tx-parsing-from-dapps-so-it-doesnt-break

100% agree. I'll tackle that follow-up PR

@walmat walmat merged commit 5b0b290 into develop Jan 7, 2025
8 checks passed
@walmat walmat deleted the @matthew/APP-2247 branch January 7, 2025 00:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants