feat(staking): [LW-8751] show pool rewards in activity #624

refi93 · 2023-10-10T08:03:13Z

Checklist

JIRA - LW-8751
Proper tests implemented
Screenshots added.

Proposed solution

Join rewardsHistory with wallet activity observable data and show it in wallet activity. The logic around showing the activity was at this point pretty complex and heavily relied on the activity being bound to a transaction which isn't the case for pool rewards which are just deposited to user's reward account at the end of the epoch. To avoid confusion down the round around that nuance, this PR also refactors the TransactionDetailBrowser component and the related "sub-components"/state in way that rewards activity is clearly split from "transactions" activity in a type-safe way

The overall diff is pretty big and I would recommend reviewing this PR primarily by commits - the first commit introduces the rewards in the history in the least "invasive" way and the subsequent commits refactor the components/state in several stages, achieving the desired split of rewards from transactions activity at the end.

Testing

Check that rewards are properly shown and check that the timestamps correspond with respective epoch starts, ideally by comparing with a block explorer
Check that wallet activity is properly loaded, not only for rewards but also for other kinds of transactions which this PR touched but there should be no functional change.

Note on designs: there are no Figma designs to validate the "Rewards" detail screen against. As per the conversation in the JIRA task, the Rewards detail screen has been implemented reusing existing elements as much as possible. Feel free to suggest improvements to the looks/layout of the Rewards detail screen

Screenshots

in case of rewards from multiple pools:

Allure report

allure-report-publisher generated test report!

smokeTests: ❌ test report for 8d20a7b9

	passed	failed	skipped	flaky	total	result
Total	31	1	0	0	32	❌

xdzurman

Will finish last couple of commits tomorrow

xdzurman · 2023-10-19T14:47:31Z

packages/core/src/ui/components/ActivityDetail/RewardsDetails.tsx

+                {t('package.core.transactionDetailBrowser.pools')}
+              </div>
+              <div className={styles.poolsList}>
+                {poolRewards?.map(({ pool, amount }) => (


Suggested change

{poolRewards?.map(({ pool, amount }) => (

{poolRewards.map(({ pool, amount }) => (

szymonmaslowski

Great work! I have just minor comments. Please address them before pushing it forward 😉

szymonmaslowski · 2023-10-20T07:55:29Z

packages/e2e-tests/src/elements/transactionDetails.ts

@@ -2,7 +2,7 @@
 import { ChainablePromiseElement } from 'webdriverio';
 import CommonDrawerElements from './CommonDrawerElements';

-class TransactionDetailsPage extends CommonDrawerElements {
+class ActivityDetailsPage extends CommonDrawerElements {


I think the filename should be also renamed

Good catch - though actually this specific class seems to concern only "transaction" details so rather than the file I'd rename the class back to its original name

szymonmaslowski · 2023-10-20T08:07:16Z

packages/core/src/ui/components/ActivityDetail/TransactionDetails.tsx

+import type { RewardsInfo } from './RewardsInfo';
+import { ActivityDetailHeaderBrowser } from './ActivityDetailHeaderBrowser';
+
+// eslint-disable-next-line @typescript-eslint/no-explicit-any


Redundant comment. It is disabled at the top already

fixed - I rather dropped the top comment in order to not needlessly relax the rule for the whole file

szymonmaslowski · 2023-10-20T08:13:35Z

packages/core/src/ui/components/ActivityDetail/TransactionDetails.tsx

@@ -0,0 +1,354 @@
+/* eslint-disable no-magic-numbers */


It's a pitty that this is recognised as a new file. I noticed that when you rename the file and make changes in it at the same time then git looses track and treats it as a new file. Try next time with making two separate commits, first for the rename and the second for the changes. I am not sure if it is worth fixing the history in this PR...

szymonmaslowski · 2023-10-20T08:24:02Z

packages/core/src/ui/components/ActivityDetail/TransactionDetails.tsx

+  txSummary?: TxSummary[];
+  coinSymbol: string;
+  tooltipContent?: string;
+  rewards?: RewardsInfo;


So this is a change compared to the original file... But I don't see usages of this property. Could you please check the props and remove those unused 🙏

besides "rewards" which are now consumed by the RewardsDetails component all other props should be relevant

szymonmaslowski · 2023-10-20T08:58:36Z

packages/core/src/ui/components/ActivityDetail/ActivityDetailHeaderBrowser.tsx

It might be relevant to rename this component to ActivityDetailHeader

szymonmaslowski · 2023-10-20T09:35:15Z

...wser-extension-wallet/src/views/browser-view/features/activity/components/ActivityDetail.tsx

+  status: ActivityStatus;
+  amountTransformer: (amount: string) => string;
+};
+const TransactionDetailsProxy = withAddressBookContext<TransactionDetailsProxyProps>(


Please move this component to dedicated file.

szymonmaslowski · 2023-10-20T09:36:02Z

...wser-extension-wallet/src/views/browser-view/features/activity/components/ActivityDetail.tsx

+  status: ActivityStatus;
+  amountTransformer: (amount: string) => string;
+};
+const TransactionDetailsProxy = withAddressBookContext<TransactionDetailsProxyProps>(


I think passing the TransactionDetailsProxyProps in the "diamond" is redundant as TS will infer the props type from passed component

szymonmaslowski · 2023-10-20T09:46:39Z

...wser-extension-wallet/src/views/browser-view/features/activity/components/ActivityDetail.tsx

+}
+
+const getTypeLabel = (type: ActivityType, t: ReturnType<typeof useTranslate>['t']) => {
+  if (type === 'rewards') return t('package.core.transactionDetailBrowser.rewards');


Please rename all the keys

Suggested change

if (type === 'rewards') return t('package.core.transactionDetailBrowser.rewards');

if (type === 'rewards') return t('package.core.activityDetails.rewards');

BTW. I noticed the the key includes package.core part while this component is placed in the browser. Could you check if that is correct? It might be either moving the translations or/and changing the key to reflect it is a browser code.

It seems all translations for the browser are prefixed by packages.core:

lace/apps/browser-extension-wallet/src/lib/translations/en.json

Line 2 in b7bfb0b

"package.core": {

so I guess this is not worth changing at the moment

szymonmaslowski · 2023-10-20T09:50:45Z

...r-extension-wallet/src/views/browser-view/features/activity/helpers/common-tx-transformer.ts

+    case Wallet.TransactionStatus.SPENDABLE:
+      return ActivityStatus.SPENDABLE;
+    default:
+      assertUnreachable(status);


Consider an object instead of a function with a switch block. Having an object IMO it is cleaner and we can ensure the typesafety, and we won't have eslint complaining about the consistent return.

szymonmaslowski · 2023-10-20T10:25:48Z

apps/browser-extension-wallet/src/stores/slices/wallet-activities-slice.ts

  UISlice,
  BlockchainProviderSlice,
  SliceCreator
 } from '../types';
 import { getAssetsInformation } from '@src/utils/get-assets-information';
+import { rewardHistoryTransformer } from '@src/views/browser-view/features/activity/helpers/reward-history-transformer';
+import { EpochNo } from '@cardano-sdk/core/dist/cjs/Cardano';


Please reexport it from @lace/cardano's Wallet object and access through it.

Or, merge this import with the one in the 9th line

using Wallet.Cardano.EpochNo seems to work as well, fixed 👍

Signed-off-by: Rafael Korbaš <[email protected]>

refi93 requested a review from a team as a October 10, 2023 08:03

refi93 marked this pull request as draft October 10, 2023 08:03

github-actions bot added the browser Changes to the browser application. label Oct 10, 2023

refi93 force-pushed the feat/lw-8315-separate-witdraw-event-on-deregistration branch 6 times, most recently from ba836f9 to 6ae38aa Compare October 10, 2023 08:40

refi93 force-pushed the feat/lw-8751-show-pool-rewards-in-activity branch 8 times, most recently from 8d20a7b to e2b174b Compare October 16, 2023 22:00

Base automatically changed from feat/lw-8315-separate-witdraw-event-on-deregistration to main October 17, 2023 09:44

github-actions bot added the e2e Changes to the e2e testing instrumentation. label Oct 17, 2023

refi93 force-pushed the feat/lw-8751-show-pool-rewards-in-activity branch 5 times, most recently from adb956e to 1b4ef33 Compare October 17, 2023 11:25

github-actions bot added the staking Changes to the staking package. label Oct 17, 2023

refi93 force-pushed the feat/lw-8751-show-pool-rewards-in-activity branch from d1e9ec4 to 7eee47d Compare October 17, 2023 14:30

refi93 added 4 commits October 18, 2023 13:13

feat(staking): include pool rewards in wallet activity

78252f4

feat(staking): expose reward amount breakdown in activity detail

083c445

refactor(all): generalize "Transaction" name to "Activity"

aa09aa6

refactor(all): rename getActivityDetails to getActivityDetail

a0dfca9

refi93 added 2 commits October 19, 2023 15:52

fix(all): fix wallet activity unit tests

5f0faff

Merge branch 'main' into feat/lw-8751-show-pool-rewards-in-activity

d0e79f5

xdzurman reviewed Oct 19, 2023

View reviewed changes

xdzurman approved these changes Oct 20, 2023

View reviewed changes

szymonmaslowski approved these changes Oct 20, 2023

View reviewed changes

refi93 added 20 commits October 20, 2023 15:07

fix(all): code review fixes

f36ac67

fix(all): make rewards in RewardsDetails component non-optional

0739235

refactor(all): remove redundant props from Activity UI components

701c284

refactor(all): rename transaction key for activity details

0537051

refactor(all): change way of importing EpochNo type

e0ae082

refactor(all): refactor transformTransactionStatus()

959aac2

fix(all): lint fixes

d6714d3

refactor(all): move TransactionDetailsProxy to separate file

28d1414

refactor(all): remove redundant ts comment

b2710c8

test(all): add reward ui component test

0ef664e

fix(all): fix crashing build

5dc1216

fix(all): fix build

734c46e

fix(all): fix type import

edc882f

fix(all): fix ci build

3accfcc

fix(all): fix ci build

6271c50

fix(all): fix ci build

5d6715f

Merge branch 'main' into feat/lw-8751-show-pool-rewards-in-activity

3349309

Signed-off-by: Rafael Korbaš <[email protected]>

fix(all): add forgotten prop to RewardsDetails

f398c32

fix(all): make rewards details time non-optional

8845a53

refactor(all): enforce spendable status for rewards details component

1a8da57

oldGreg5 approved these changes Oct 26, 2023

View reviewed changes

refi93 added 2 commits October 27, 2023 10:02

Merge branch 'main' into feat/lw-8751-show-pool-rewards-in-activity

b24bd8c

Signed-off-by: Rafael Korbaš <[email protected]>

Merge branch 'main' into feat/lw-8751-show-pool-rewards-in-activity

74a9b66

Signed-off-by: Rafael Korbaš <[email protected]>

przemyslaw-wlodek merged commit 6e9495b into main Oct 30, 2023
6 of 7 checks passed

przemyslaw-wlodek deleted the feat/lw-8751-show-pool-rewards-in-activity branch October 30, 2023 09:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(staking): [LW-8751] show pool rewards in activity #624

feat(staking): [LW-8751] show pool rewards in activity #624

refi93 commented Oct 10, 2023 •

edited

Loading

xdzurman left a comment

xdzurman Oct 19, 2023

szymonmaslowski left a comment

szymonmaslowski Oct 20, 2023

refi93 Oct 20, 2023

szymonmaslowski Oct 20, 2023

refi93 Oct 24, 2023

szymonmaslowski Oct 20, 2023

szymonmaslowski Oct 20, 2023

refi93 Oct 20, 2023

szymonmaslowski Oct 20, 2023

szymonmaslowski Oct 20, 2023

szymonmaslowski Oct 20, 2023

szymonmaslowski Oct 20, 2023

refi93 Oct 20, 2023

szymonmaslowski Oct 20, 2023

szymonmaslowski Oct 20, 2023

refi93 Oct 20, 2023 •

edited

Loading

	{poolRewards?.map(({ pool, amount }) => (
	{poolRewards.map(({ pool, amount }) => (

	if (type === 'rewards') return t('package.core.transactionDetailBrowser.rewards');
	if (type === 'rewards') return t('package.core.activityDetails.rewards');

feat(staking): [LW-8751] show pool rewards in activity #624

feat(staking): [LW-8751] show pool rewards in activity #624

Conversation

refi93 commented Oct 10, 2023 • edited Loading

Checklist

Proposed solution

Testing

Screenshots

Allure report

xdzurman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szymonmaslowski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refi93 Oct 20, 2023 • edited Loading

Choose a reason for hiding this comment

refi93 commented Oct 10, 2023 •

edited

Loading

refi93 Oct 20, 2023 •

edited

Loading