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

feat(mobile): staking view-only #14561

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

feat(mobile): staking view-only #14561

wants to merge 1 commit into from

Conversation

Nodonisko
Copy link
Contributor

Description

As much as possible coin agnostic staking implementation for mobile app.

Known issue:

  1. Staking is not reflected in graph - could be solved probably but we are either fine with that or we will do it in another PR
  2. Toast that should show when staking is clicked in Receive/Send flow is behind modal (this will require refactor of BottomSheet to fix it or we should come up with different solution)
  3. Missing desktop icon on Staking Details (we should introduce new icon set ASAP)

Related Issue

Resolve #12187

Screenshots:

@Nodonisko Nodonisko requested a review from a team as a code owner September 26, 2024 16:13
@Nodonisko Nodonisko changed the title temp feat(mobile): staking view-only Sep 26, 2024
@Nodonisko Nodonisko mentioned this pull request Sep 26, 2024
Closed
@vytick vytick added the mobile Suite Lite issues and PRs label Sep 26, 2024
vytick
vytick reviewed Sep 26, 2024
View reviewed changes
Copy link
Contributor

@vytick vytick left a comment

Choose a reason for hiding this comment

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

I need to test it more, but it looks nice from code :)

suite-native/accounts/src/components/AccountsList/AccountsListStakingItem.tsx Show resolved Hide resolved
suite-native/atoms/src/AlertBox.tsx Show resolved Hide resolved
suite-native/atoms/src/AlertBox.tsx Outdated Show resolved Hide resolved
suite-native/staking/src/selectors.ts Show resolved Hide resolved
PeKne
PeKne requested changes Sep 27, 2024
View reviewed changes
suite-native/accounts/src/components/AccountSectionTitle.tsx Outdated
@@ -36,6 +37,12 @@ export const AccountSectionTitle: React.FC<AccountSectionTitleProps> = ({
adjustsFontSizeToFit
value={fiatBalance}
/>
<CryptoAmountFormatter
value={getCryptoBalanceWithStaking(account)}
Copy link
Contributor

Choose a reason for hiding this comment

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

What about creating selector for 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.

good idea, done

suite-native/accounts/src/components/AccountsList/AccountsListItem.tsx Outdated
const shouldShowAccountLabel = !doesCoinSupportTokens || hideTokens;
const shouldShowTokenBadge = accountHasAnyTokens && hideTokens;
const shouldShowAccountLabel = !doesCoinSupportTokens || hideBadges;
const shouldShowTokenBadge = accountHasAnyTokens && hideBadges;
Copy link
Contributor

Choose a reason for hiding this comment

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

The naming is confusing. I would expect it to be

Suggested change
const shouldShowTokenBadge = accountHasAnyTokens && hideBadges;
const shouldShowTokenBadge = accountHasAnyTokens && !hideBadges;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, it was confusing I renamed it and it should be more clear now

suite-native/accounts/src/components/AccountsList/AccountsListStakingItem.tsx Outdated
Comment on lines 18 to 21
account,
onPress,
stakingCryptoBalance,
hasBackground,
isFirst,
isLast,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: onPress, hasBackground, isFirst, isLast does not have to be explicitly named here. Instead spreaded ...otherProps value could be used and passed in a single line to AccountsListItemBase.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

suite-native/accounts/src/components/AccountsList/AccountsListStakingItem.tsx Outdated
showDivider={!isLast}
onPress={onPress}
icon={<RoundedIcon name="piggyBankFilled" color="iconSubdued" />}
title="Staking"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add translation string for this

suite-common/wallet-utils/src/stakingUtils.ts Outdated
export const getEverstakePool = (account?: Account) => {
if (account?.networkType !== 'ethereum') {
return undefined;
export const getEverstakePool = (account?: Account | AccountInfo) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

AccountInfo should be enough since it is a subset of Account.

Suggested change
export const getEverstakePool = (account?: Account | AccountInfo) => {
export const getEverstakePool = (account?: AccountInfo) => {

If you gonna change this, please change it across this whole file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed this type change completely, it is not needed.

suite-native/module-staking-management/src/components/StakingBalancesOverviewCard.tsx Outdated
}));

const separatorStyle = prepareNativeStyle(utils => ({
borderBottomWidth: 1,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
borderBottomWidth: 1,
borderBottomWidth: utils.borders.width,s.small
``

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

suite-native/module-staking-management/src/components/StakingInfo.tsx Outdated
import { StakingBalancesOverviewCard } from './StakingBalancesOverviewCard';
import { StakingUnavailableBottomSheet } from './StakingUnavailableBottomSheet';

const sectiontStyle = prepareNativeStyle(utils => ({
Copy link
Contributor

Choose a reason for hiding this comment

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

typo

Suggested change
const sectiontStyle = prepareNativeStyle(utils => ({
const sectionStyle = prepareNativeStyle(utils => ({

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

suite-native/module-staking-management/src/components/StakingUnavailableBottomSheet.tsx Outdated
</Text>

<Button
onPress={() => handleToggleBottomSheet(false)}
Copy link
Contributor

Choose a reason for hiding this comment

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

handleToggleBottomSheet(true) is never used in this file. The argument seems to be redundant in context of this file. What about passing () => handleToggleBottomSheet(false) from parent directly?

Suggested change
onPress={() => handleToggleBottomSheet(false)}
onPress={handleToggleBottomSheet}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good idea, fixed

suite-native/staking/src/ethereum/ethereumSelectors.ts Outdated
@@ -0,0 +1,101 @@
import { NetworkSymbol } from '@suite-common/wallet-config';
Copy link
Contributor

Choose a reason for hiding this comment

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

What about naming this file ethereumStakingSelectors.ts for better context?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yea, sounds nice

suite-native/staking/src/selectors.ts Outdated
case 'eth':
case 'thol':
case 'tsep':
return selectVisibleDeviceEthereumAccountsWithStakingByNetworkSymbol(state, 'eth');
Copy link
Contributor

Choose a reason for hiding this comment

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

make sure to return bool for type safety.

Suggested change
return selectVisibleDeviceEthereumAccountsWithStakingByNetworkSymbol(state, 'eth');
return A.isNotEmpty(selectVisibleDeviceEthereumAccountsWithStakingByNetworkSymbol(state, 'eth'));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@Nodonisko Nodonisko force-pushed the feat/mobile-staking branch 4 times, most recently from 097a050 to ee339ad Compare September 27, 2024 11:08
vytick
vytick approved these changes Sep 27, 2024
View reviewed changes
Copy link
Contributor

@vytick vytick left a comment

Choose a reason for hiding this comment

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

good job 👌

suite-native/accounts/src/components/AccountSelectBottomSheet.tsx
} else {
showToast({
variant: 'warning',
message: 'Staking is not available in this context.',
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: If we expect this to ne visible to user, it would be good to add to intl

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

@PeKne PeKne PeKne requested changes

@vytick vytick vytick approved these changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
mobile Suite Lite issues and PRs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Reflect balances & staking in the app
3 participants
@Nodonisko @vytick @PeKne