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.

Sign up for GitHub

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

Jump to bottom

Enable Solana on mobile via message system feature #15700

Merged
merged 2 commits into from
Dec 10, 2024
Merged

Enable Solana on mobile via message system feature #15700

merged 2 commits into from
Dec 10, 2024

Conversation

vytick
Copy link
Contributor

@vytick vytick commented Dec 2, 2024 •
edited
Loading

Resolve #15613

@vytick vytick added mobile Suite Lite issues and PRs solana labels Dec 2, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 2, 2024 •
edited
Loading

🚀 Expo preview is ready!

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

Learn more about 𝝠 Expo Github Action

@vytick vytick mentioned this pull request Dec 2, 2024
Draft
@vytick vytick added the message-system Updates in secure message system config label Dec 2, 2024
@vytick vytick force-pushed the feat/native/solana-messages-featureflag branch from 14d6795 to 1be1926 Compare December 3, 2024 13:30
@trezor-ci trezor-ci force-pushed the feat/native/solana-messages-featureflag branch from c5453ca to 3c06bab Compare December 3, 2024 15:34
@vytick vytick force-pushed the feat/native/solana-messages-featureflag branch 2 times, most recently from 6c2e21c to 630434f Compare December 4, 2024 13:22
@vytick vytick marked this pull request as ready for review December 4, 2024 13:38
@vytick vytick requested review from a team, tomasklim and matejkriz as code owners December 4, 2024 13:38
@trezor trezor deleted a comment from github-actions bot Dec 4, 2024
@matejkriz matejkriz self-assigned this Dec 6, 2024
@vytick vytick force-pushed the feat/native/solana-messages-featureflag branch 3 times, most recently from 59d8d19 to 0c83f3d Compare December 9, 2024 19:48
PeKne
PeKne reviewed Dec 10, 2024
View reviewed changes
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.

The code looks good 👍

suite-native/message-system/src/components/KillswitchMessageScreen.tsx Outdated
const { applyStyle } = useNativeStyles();

if (!firstFeatureMessage) return null;
const killswitch = A.head(useSelector(selectActiveKillswitchMessages));
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: What about changing this selector to selectFirstActiveKillswitchMessage and put the A.head inside?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

49a065c

Copy link
Member

Choose a reason for hiding this comment

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

Even selectActiveKillswitchMessage (without the First) sounds good to me for this (only one can be active...)

matejkriz
matejkriz reviewed Dec 10, 2024
View reviewed changes
suite-native/feature-flags/src/featureFlagsSlice.ts Outdated
FeatureFlag.IsRegtestEnabled,
FeatureFlag.IsSolanaEnabled,
FeatureFlag.IsSolanaEnabledByRemote,
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if it makes sense to mix the feature flag from message system with local feature flags in one slice 🤔 It seems like a duplication of the state to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed

@vytick vytick force-pushed the feat/native/solana-messages-featureflag branch from 49a065c to d85fbdf Compare December 10, 2024 14:10
vytick
vytick commented Dec 10, 2024
View reviewed changes
suite-native/module-accounts-management/src/selectors.ts
const isSolanaSendEnabled = selectIsFeatureFlagEnabled(state, FeatureFlag.IsSolanaSendEnabled);

if (isSolanaSendEnabled && networkType === 'solana') return true;
if (networkType !== 'cardano' || isCardanoSendEnabled) return true;
Copy link
Contributor Author

@vytick vytick Dec 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

send for all the other networkTypes is already enabled, and we will enable sending solana at the same time as we start supporting it. So there is no need to check anything else but cardano FF

Copy link
Contributor

Choose a reason for hiding this comment

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

makes sense to simplify that 👍

matejkriz
matejkriz reviewed Dec 10, 2024
View reviewed changes
suite-native/feature-flags/src/featureFlagsSlice.ts Outdated
@@ -44,6 +41,12 @@ export const featureFlagsSlice = createSlice({
toggleFeatureFlag: (state, { payload }: PayloadAction<{ featureFlag: FeatureFlag }>) => {
state[payload.featureFlag] = !state[payload.featureFlag];
},
setFeatureFlag: (
Copy link
Member

Choose a reason for hiding this comment

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

setFeatureFlag is not used anymore

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

matejkriz
matejkriz approved these changes Dec 10, 2024
View reviewed changes
Copy link
Member

@matejkriz matejkriz left a comment

Choose a reason for hiding this comment

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

Great, it's much simpler now and works correctly, thanks! Just please remove unused setFeatureFlag 🙏

@trezor-ci trezor-ci force-pushed the feat/native/solana-messages-featureflag branch from 410617d to 083e638 Compare December 10, 2024 14:47
@trezor trezor deleted a comment from github-actions bot Dec 10, 2024
@vytick
Copy link
Contributor Author

vytick commented Dec 10, 2024

Notes for QA @STew790 : There are two stages of testing, 1. can be done right away, but lets sync up on testing 2.

  1. App behaviour:
  • By default Solana should be disabled
  • Solana cant be imported nor is discovered or added new account to device
  • enabling local FF should enable it
  1. needs to deploy message to develop with enabling Solana and afterwards. App behaviour:
  • By default Solana should be enabled
  • User should be able to import, discover add and send Solana
  • local FF is irrelevant in this case

@vytick vytick merged commit 7513ee0 into develop Dec 10, 2024
28 checks passed
@vytick vytick deleted the feat/native/solana-messages-featureflag branch December 10, 2024 15:18
@STew790
Copy link
Contributor

STew790 commented Dec 10, 2024

QA OK
Tested both behaviors, both worked as expected.
Tested also disabling and works also as expected. 🎉

@STew790 STew790 mentioned this pull request Dec 10, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PeKne PeKne PeKne approved these changes

@matejkriz matejkriz matejkriz approved these changes

@tomasklim tomasklim Awaiting requested review from tomasklim tomasklim is a code owner

Assignees

@matejkriz matejkriz

Labels
message-system Updates in secure message system config mobile Suite Lite issues and PRs solana
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Prepare feature flag for Solana in message-system
4 participants
@vytick @STew790 @matejkriz @PeKne