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

Mobile: sending Solana #15533

Merged
merged 3 commits into from
Dec 3, 2024
Merged

Mobile: sending Solana #15533

merged 3 commits into from
Dec 3, 2024

Conversation

vytick
Copy link
Contributor

@vytick vytick commented Nov 22, 2024

Enables sending Solana and tokens including basic error handling for failed send. No custom fees are allowed (same as desktop).

Related Issue

Resolve #15497

Screenshots:

happy path:

happy_path.mp4

unhappy path:

unhappy_path.mp4

Copy link

github-actions bot commented Nov 22, 2024

🚀 Expo preview is ready!

  • Project → trezor-suite-preview
  • Platforms → android, ios
  • Scheme → trezorsuitelite
  • Runtime Version → 16
  • More info

Learn more about 𝝠 Expo Github Action

@trezor-ci trezor-ci force-pushed the feat/native/send-solana branch from 1edfd80 to 13b6882 Compare November 22, 2024 22:00
@trezor trezor deleted a comment from github-actions bot Nov 22, 2024
@vytick vytick added connect Connect API related (ie. fee calculation) mobile Suite Lite issues and PRs solana labels Nov 22, 2024
@vytick vytick marked this pull request as ready for review November 22, 2024 22:03
minFee: -1, // unused
maxFee: -1, // unused
minFee: 25000,
maxFee: 1000000000,
Copy link
Contributor

Choose a reason for hiding this comment

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

how come that suite didn't need this? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is probably no validation on this. And the fees are determined automatically by fetching the on chain data

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, good with me 👍

Copy link
Contributor

@PeKne PeKne left a comment

Choose a reason for hiding this comment

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

If I get to the state of timeouted transaction once, I am unable the send any other SOL transaction again. It always resolves in the timeout error. Only that helps is restart of the app. There is probably something rotting in the redux state.

Zaznam.obrazovky.2024-11-25.v.12.39.36.mov

@vytick vytick force-pushed the feat/native/send-solana branch 2 times, most recently from 853cba5 to 195e5c2 Compare December 2, 2024 12:06
Copy link

socket-security bot commented Dec 2, 2024

🚨 Potential security issues detected. Learn more about Socket for GitHub ↗︎

To accept the risk, merge this PR and you will not be notified again.

Alert Package NoteSourceCI
Network access npm/[email protected] 🚫

View full report↗︎

Next steps

What is network access?

This module accesses the network.

Packages should remove all network access that is functionally unnecessary. Consumers should audit network access to ensure legitimate use.

Take a deeper look at the dependency

Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev.

Remove the package

If you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency.

Mark a package as acceptable risk

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of ecosystem/package-name@version specifiers. e.g. @SocketSecurity ignore npm/[email protected] or ignore all packages with @SocketSecurity ignore-all

// Subscribe to blocks for Solana, since they are not fetched globally
// this is needed for correct Solana fee estimation
if (account && account.networkType === 'solana') {
TrezorConnect.blockchainSubscribe({
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: maybe create variable for that params and use in both calls?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a copy of what @martykan used for desktop.

Copy link
Contributor

Choose a reason for hiding this comment

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

That doesn't mean it coudn't be improved a bit :D

@vytick vytick force-pushed the feat/native/send-solana branch from 076fb5a to 84bc3a2 Compare December 2, 2024 22:08
@vytick vytick requested a review from PeKne December 3, 2024 10:27
@vytick vytick force-pushed the feat/native/send-solana branch from 84bc3a2 to 010a539 Compare December 3, 2024 15:47
@trezor trezor deleted a comment from github-actions bot Dec 3, 2024
@trezor trezor deleted a comment from github-actions bot Dec 3, 2024
@vytick vytick merged commit cda066a into develop Dec 3, 2024
67 of 69 checks passed
@vytick vytick deleted the feat/native/send-solana branch December 3, 2024 20:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
connect Connect API related (ie. fee calculation) mobile Suite Lite issues and PRs solana
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Send Solana + Tokens
4 participants