Operator mapping - code cleaning #223

michalsmiarowski · 2022-10-03T07:15:08Z

Closes #232 (64677b3)
Closes #255 (ae4231d)

Operator mapping code cleaning

Main changes:

rearrange the operator mapping related data in redux store
adds operator and staking provider addresses as props to the Operator Mapping related modals and OperatorAddressMappingCard
removes unnecessary shouldDisplayMapOperatorToStakingProviderModal effect
fix some of the review comments from Map operator to staking provider feature #199
fix a bug that caused the MapOperatorToStakingProvider modal to not be displayed when we changed accounts in MetaMask
update text in operator mapping modal (Closes: Operator Mapping Modal Copy needs an update #232)

TODO in the future:

display Operator Mapping Card when the operator is mapped - Display Operator Mapping Card when operator is mapped #259
display Operator Mapping Card and the modals related to it after submitting a stake (without the need to refresh)

Change details

rearrange the operator mapping related data in redux store

Previously we keep the data needed to operator mapping in the redux store in this format:

(1) rolesOf property in connected-account slice:

{
address: string,
  rolesOf: {
    owner: string,
    authorizer: string,
    beneficiary: string, 
}

This indicated if the currently logged account is used as a staking provider in other stakes. We fetched the data from the contract.

(2) mappedOperator property in staking-applications slice:

{
  tbtc: {
    parameter: (...),
    stakingProviders: (...),
    mappedOperator: {
      isInitialFetchDone: boolean,
      isFetching: boolean,
      error: string,
      data: string,
  },
  randomBeacon: {
    parameter: (...),
    stakingProviders: (...),
    mappedOperator: {
      isInitialFetchDone: boolean,
      isFetching: boolean,
      error: string,
      data: string,
  }
}

This stored the data related to currently logged staking provider and it's mapped operator for each application. This could wrongly indicate that the data is more related to the staking app itself (since it was in the staking-application slice) when in reality it was more related to currently connected account.

At first the idea was to not store this data in redux store at all, but unfrotunately this didn't went as smooth as I thogught and I think we must store them mainly to properly display OperatorAddressMappingCard in the staking page for all staking providers that don't have the operator mapped. I've encoutered problems with implementing it without using the store when the staking provider !=== owner.

This is why i decided to go back to keeping the data in the redux store (you can see all this in the commit history) but in better (and i think simpler) form.

Right now we will store this data in account slice and we ditched the whole rolesOf property which was object, to just isStakingProvider which is boolean value. After the changes the connected-account is renamed to account and looks like this:

{
  address: string,
  isStakingProvider: boolean,
  operatorMapping: {
    data: {
      tbtc: string,
      randomBeacon: string,
    },
    isFetching: boolean,
    isInitialFetchDone: boolean,
  },
}

adds operator and staking provider addresses as props to the Operator Mapping related modals and `OperatorAddressMappingCard`

For these components we are adding a props that contains operator address and staking provider address:

OperatorAddressMappingCard
MapOperatorToStakingProviderModal
MapOperatorToStakingProviderSuccessModal
MapOperatorToStakingProviderConfirmationModal
MapOperatorToStakingProviderForm

removes unnecessary shouldDisplayMapOperatorToStakingProviderModal effect

This effect was unnecessary. Instead of using predicate (from redux-toolkit) when calling displayMapOperatorToStakingProviderModalEffect we now use actionCreator (also from redux-toolkit) to call this effect on operatorMappingInitialFetchDone action call which in turn is called by setStakes action. This way we are sure we have all needed that in redux store and we can use them in displayMapOperatorToStakingProviderModalEffect

fix some of the review comments from #199

Most comment were either fixed by itself during the refactor process (the part of code that was commented does no longer exist) and other ones I've tried to fix. For some I've added comments there to start a discussion.

fix a bug that caused the `MapOperatorToStakingProvider` modal to not be displayed when we changed accounts in MetaMask

We are setting the isSuccessfullLoginModalClosed flag to false when resetting the store now, because the reset happend when we change the accounts and the login modal is not displayed in such case. We also set isMappingOperatorToStakingProviderModalClosed flag back to false in this case.

Additional note: The modal queue was also moved in the store to modal prop since it's related to modal. In the furue we are planning to create a real modal queue.

update text in operator mapping modal

Update text based on Figma designs in OperatorAddressMappingCard component and in MapOperatorToStakingProviderModal. For more details see #232

We decided that we don't have to store the data related to operator mapping in the redux store. This commit adds that. I've also commented out `OperatorAddressMappingCard`. This card will be addressed in the future commits when working on subcribtion to "OperatorRegistered" event

There was a bug where when you close the login modal too quick, before `setStakes` action was dispatched, then the effect got blocked on `await listenerApi.condition` and was waiting for another action to dispatch and there were not any at that particular moment. To fix that I've added a condition to check if login modal was closed, if not, then we wait with the `condition()` function since the `displayMapOperatorToStakingProviderModalEffect` effect is run on a `setStakes` action, so we have all needed data fetcher already.

Fix displaying operator mapping modal when changins accouns. The display of this modal is based on the condition that the user closed succesfull login modal. We store this flag in `modalQueue` redux store. When changing accounts this modal is not displayed thus the flag is not set. To fix that we wil set this flag to true when resetting redux store action is called.

Adds data about operator mapping to the connectedAccount slice: { isUsedAsStakingProvider: boolean mappedOperator: { tbtc: string randomBeacon: string } } This data is neede to properly display the OperatorMappingCard that is on the Staking page.

Gets the proper data from redux store and conditionally display OperatorAddressMappingCard in staking page

Move operatorMapped event to connectedAddress slice and rename it to operatorRegistered to indicate it's relation with `OperatorRegistered` event.

`stakingProvider` prop was not used in `MapOperatorToStakingProviderModal` so we are removing it.

Use mappedOperators data from the store instead of calling the `getMappedOperatorsForStakingProvider` method from threshold lib in `useRegisterMultipleOperatorsTransaction` hook.

github-actions · 2022-10-03T07:38:04Z

Preview uploaded to https://preview.dashboard.test.threshold.network/operator-mapping-code-cleaning/index.html.

Operator Address Mapping Card should be displayed when tbtc mapped operator is AddressZero OR (not AND) random beacon mapped operator is AddressZero. This commit fixes that.

Move the store related to modal queue inside `modal` property since it's related to modal.

- move provider address to the top - change `Staking Provider` -> `Provider Address` - change `Operator` -> `Operator Address` - Display `View transaction 1 and transaction 2` in one row instead of two separate rows foe each app.

The operator mapping modal and new apps to authorize modal were not displayed properly when changin accounts in MetaMask. This commit fixes that.

Property `isFetching` was wrongly set to true and stays at this value when the connected account was not a staking provider. This commit fixes that.

github-actions · 2022-10-05T12:39:19Z

Preview uploaded to https://preview.dashboard.test.threshold.network/operator-mapping-code-cleaning/index.html.

georgeweiler · 2022-10-05T16:58:23Z

src/components/Modal/MapOperatorToStakingProviderConfirmationModal/index.tsx


-  const operatorMappedToStakingProviderHelpers =


why remove this helper hook? seems like it saved some duplicate code. What if we added the tbtc & random beacon addresses as parameters so we don't have to rewrite the logic?

This will be addressed in #278

georgeweiler · 2022-10-05T16:59:44Z

src/components/Modal/MapOperatorToStakingProviderModal/MapOperatorToStakingProviderForm.tsx

@@ -69,14 +73,14 @@ const MapOperatorToStakingProviderForm = withFormik<
        }
        if (
          isOperatorMappedOnlyInRandomBeacon &&
-          values.operator !== operatorMappedRandomBeacon
+          values.operator !== mappedOperatorRandomBeacon
        ) {
          validationMsg =
            "The operator address doesn't match the one used in tbtc app"


let's use tBTC

Good catch! Fixed in a1810ef

- use `isSameEthAddress` function in some conditions instead of `!==` - move large part of the validation to a new `validateInputtedOperatorAddress` function

Use `isAddressZero` instead of `isZeroAddress`

setAccountAddress -> walletConnected

github-actions · 2022-10-17T09:57:14Z

Preview uploaded to https://preview.dashboard.test.threshold.network/operator-mapping-code-cleaning/index.html.

isUsedAsStakingProvider -> isStakingProvider Also move it outside of mappedOperators object and keep it in the root of the account slice.

We don't need mapped operator in `account` slice because we already have `operatorMapping` property.

github-actions · 2022-10-19T08:31:29Z

Preview uploaded to https://preview.dashboard.test.threshold.network/operator-mapping-code-cleaning/index.html.

Rename the `setFetchingOperatorMapping` action to `fetchingOperatorMapping` and set the fetching to true automatically.

github-actions · 2022-10-19T13:26:42Z

Preview uploaded to https://preview.dashboard.test.threshold.network/operator-mapping-code-cleaning/index.html.

github-actions · 2022-10-19T13:34:10Z

Preview uploaded to https://preview.dashboard.test.threshold.network/operator-mapping-code-cleaning/index.html.

r-czajkowski

LGTM!

@georgeweiler

@georgeweiler is on vacation, all his requested changes were addressed and we have an approval from @r-czajkowski

michalsmiarowski added 8 commits October 2, 2022 21:50

Add operatorMappingData to the store

ca8a09b

Adds data about operator mapping to the connectedAccount slice: { isUsedAsStakingProvider: boolean mappedOperator: { tbtc: string randomBeacon: string } } This data is neede to properly display the OperatorMappingCard that is on the Staking page.

Fix displaying OperatorAddressMappingCard

c1694a0

Gets the proper data from redux store and conditionally display OperatorAddressMappingCard in staking page

Rename and move operatorMapped event

6b98f55

Move operatorMapped event to connectedAddress slice and rename it to operatorRegistered to indicate it's relation with `OperatorRegistered` event.

Remove unused prop

000ef3d

`stakingProvider` prop was not used in `MapOperatorToStakingProviderModal` so we are removing it.

Use mappedOperators data from the store

04e6e42

Use mappedOperators data from the store instead of calling the `getMappedOperatorsForStakingProvider` method from threshold lib in `useRegisterMultipleOperatorsTransaction` hook.

michalsmiarowski added the 💎 components label Oct 3, 2022

michalsmiarowski mentioned this pull request Oct 3, 2022

Fix displaying new apps to authorize modal #225

Merged

michalsmiarowski added 2 commits October 4, 2022 13:46

Fix displaying OperatorAddressMappingCard

2d66a01

Operator Address Mapping Card should be displayed when tbtc mapped operator is AddressZero OR (not AND) random beacon mapped operator is AddressZero. This commit fixes that.

Refactor modalQueue state

007f883

Move the store related to modal queue inside `modal` property since it's related to modal.

r-czajkowski added this to the v1.3.1 milestone Oct 5, 2022

michalsmiarowski added 7 commits October 5, 2022 13:00

Refactor map operator success modal

73490d2

- move provider address to the top - change `Staking Provider` -> `Provider Address` - change `Operator` -> `Operator Address` - Display `View transaction 1 and transaction 2` in one row instead of two separate rows foe each app.

Fix imports

008179b

Use useAppDispatch and useAppSelector hooks

2e665b4

Fix displaying modals when changing accounts

2cf5d87

The operator mapping modal and new apps to authorize modal were not displayed properly when changin accounts in MetaMask. This commit fixes that.

Fix isFetching prop for operator address mapping

497617a

Property `isFetching` was wrongly set to true and stays at this value when the connected account was not a staking provider. This commit fixes that.

Update comment

e381413

Merge branch 'main' into operator-mapping-code-cleaning

4f160ef

michalsmiarowski marked this pull request as ready for review October 5, 2022 12:19

michalsmiarowski requested review from r-czajkowski and georgeweiler October 5, 2022 12:19

michalsmiarowski assigned georgeweiler and r-czajkowski Oct 5, 2022

michalsmiarowski mentioned this pull request Oct 5, 2022

Add success state to OperatorAddressMappingCard #261

Merged

1 task

georgeweiler reviewed Oct 5, 2022

View reviewed changes

michalsmiarowski added 5 commits October 17, 2022 00:31

Simplify MapOperatorToStakingProviderForm

7ed1d17

- use `isSameEthAddress` function in some conditions instead of `!==` - move large part of the validation to a new `validateInputtedOperatorAddress` function

Fix isAddressZero import

009344d

Use `isAddressZero` instead of `isZeroAddress`

Change setAccountAddress action name

c5453fd

setAccountAddress -> walletConnected

Remove unnecessary variables

2fd7154

Remove unnecessary keys from ListItem

676058f

michalsmiarowski requested review from r-czajkowski and georgeweiler October 17, 2022 09:41

pdyraga assigned michalsmiarowski and unassigned georgeweiler and r-czajkowski Oct 17, 2022

michalsmiarowski added 3 commits October 19, 2022 08:01

Rename isUsedAsStakingProvider prop in store

f754bc4

isUsedAsStakingProvider -> isStakingProvider Also move it outside of mappedOperators object and keep it in the root of the account slice.

Remove mappedOperators from store

390eaea

We don't need mapped operator in `account` slice because we already have `operatorMapping` property.

Refactor setMappingOperator reducer

7cf4736

michalsmiarowski added 2 commits October 19, 2022 15:07

Remove setFetchingOperatorMapping

111117d

Rename the `setFetchingOperatorMapping` action to `fetchingOperatorMapping` and set the fetching to true automatically.

Fix validation in OperatorToStakingProvider form

96105de

r-czajkowski approved these changes Oct 21, 2022

View reviewed changes

r-czajkowski enabled auto-merge October 21, 2022 12:32

michalsmiarowski requested review from georgeweiler and removed request for georgeweiler October 24, 2022 08:00

r-czajkowski requested review from georgeweiler and r-czajkowski October 24, 2022 08:59

r-czajkowski approved these changes Oct 24, 2022

View reviewed changes

r-czajkowski merged commit 2932ad4 into main Oct 24, 2022

r-czajkowski deleted the operator-mapping-code-cleaning branch October 24, 2022 09:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Operator mapping - code cleaning #223

Operator mapping - code cleaning #223

michalsmiarowski commented Oct 3, 2022 •

edited

Loading

github-actions bot commented Oct 3, 2022

github-actions bot commented Oct 5, 2022

georgeweiler Oct 5, 2022

michalsmiarowski Oct 21, 2022

georgeweiler Oct 5, 2022

michalsmiarowski Oct 10, 2022 •

edited

Loading

github-actions bot commented Oct 17, 2022

github-actions bot commented Oct 19, 2022

github-actions bot commented Oct 19, 2022

github-actions bot commented Oct 19, 2022

r-czajkowski left a comment

Operator mapping - code cleaning #223

Operator mapping - code cleaning #223

Conversation

michalsmiarowski commented Oct 3, 2022 • edited Loading

Operator mapping code cleaning

Main changes:

Change details

rearrange the operator mapping related data in redux store

adds operator and staking provider addresses as props to the Operator Mapping related modals and OperatorAddressMappingCard

removes unnecessary shouldDisplayMapOperatorToStakingProviderModal effect

fix some of the review comments from #199

fix a bug that caused the MapOperatorToStakingProvider modal to not be displayed when we changed accounts in MetaMask

update text in operator mapping modal

github-actions bot commented Oct 3, 2022

github-actions bot commented Oct 5, 2022

georgeweiler Oct 5, 2022

Choose a reason for hiding this comment

michalsmiarowski Oct 21, 2022

Choose a reason for hiding this comment

georgeweiler Oct 5, 2022

Choose a reason for hiding this comment

michalsmiarowski Oct 10, 2022 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Oct 17, 2022

github-actions bot commented Oct 19, 2022

github-actions bot commented Oct 19, 2022

github-actions bot commented Oct 19, 2022

r-czajkowski left a comment

Choose a reason for hiding this comment

michalsmiarowski commented Oct 3, 2022 •

edited

Loading

adds operator and staking provider addresses as props to the Operator Mapping related modals and `OperatorAddressMappingCard`

fix a bug that caused the `MapOperatorToStakingProvider` modal to not be displayed when we changed accounts in MetaMask

michalsmiarowski Oct 10, 2022 •

edited

Loading