-
Notifications
You must be signed in to change notification settings - Fork 28
MAS UI <-> data access layer integration #178
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
Conversation
Move function that creates the Threshold lib instance to a common file to use it in other places.
Add a common type that represents a fetching state: - isFetching- fetching indicator, - error- stores the error message.
A Redux middleware that lets you define "listener" entries that contain an "effect" callback with additional logic, and a way to specify when that callback should run based on dispatched actions or state changes. Example pattern is the one off listener for fetching data only once.
Thanks to this we can use the same interface but with different number type. For eg. in redux we want to store numbers as string because the `BigNumber` is not serializable.
In order not to lose context.
Create applications slices and listener effects. These two effects are one of listeners so they fetch data only once. Thanks to that we can reduce the number of calls to the ethereum node.
It's better to create typed versions of the `useDispatch` and `useSelector` hooks for usage in your application. This is important for a couple reasons: - for `useSelector`, it saves you the need to type `(state: RootState)` every time, - for `useDispatch`, the default `Dispatch` type does not know about thunks. In order to correctly dispatch thunks, you need to use the specific customized `AppDispatch` type from the store that includes the thunk middleware types, and use that with `useDispatch`. Adding a pre-typed `useDispatch` hook keeps you from forgetting to import `AppDispatch` where it's needed.
Create selectors that returns the application state by name and application data for a given app and given staking provider.
Display data from redux store in application cards.
Preview uploaded to https://preview.dashboard.test.threshold.network/mas-integration/index.html. |
Preview uploaded to https://preview.dashboard.test.threshold.network/mas-integration/index.html. |
To be able to create a contract instance with a signer to sign transaction. Also add `useEffect` to the `ThresholdContext` to update the provider if the user connects to a wallet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First part of review done:
src/store/applications/slice.ts
Outdated
randomBeacon: ApplicationState | ||
} | ||
|
||
export type AppName = "tbtc" | "randomBeacon" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some thoughts - the "tbtc" app is actually "tbtc-v2", but we display it in UI as "tbtc" (in some cases). We also use tbtc (v1) lib in our threshold app (I see that we for example store the balance of tbtc [v1] in our store, but i can't find if we use it, or display it somewhere in our app).
I'm little afraid that it can get messy and I would consider to keep the tbtc
names that are related to tbtc-v2
and that are not displayed in the UI as tbtc-v2
. What to you think?
If you agree then for example I would change this line to:
export type AppName = "tbtc" | "randomBeacon" | |
export type AppName = "tbtc-v2" | "randomBeacon" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In terms of threshold staking applications, we have only tbtc v2 app so I think from code's/T dapp's perspective is ok to use tbtc
w/o v-2
because it's obvious that only tbtc-v2 works on threshold network. So maybe let's use suffix for tbtc-v1
? What do you think? We only added the tbtc v1
token to the TokenContext
to get the token price in usd and calculate tvl.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@r-czajkowski I think we might do it that way. It's still might get confusing since we use the terms tbtc
and tbtc-v2
outside of the project, but since we don't use tbtc-v1
that much in the project (as you said) I think your solution is also very good 👍
src/pages/Staking/index.tsx
Outdated
useEffect(() => { | ||
dispatch(applicationsSlice.actions.getSupportedApps({})) | ||
}, [dispatch]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does that mean that we are gonna fetch the app data only on Staking
page? We've got a banner on overview page that says about the app authorization and which let the user choose the stake he wants to authorize. We might need this data there too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep good point. Can we address it in a separate PR once we merge all necessary components/UI for MAS?
Update `selectAppByStakingProvider` to return additional data instead of computing them in component scope. Here we add: - `isAuthorized`- informs if a given app has been authrozied by authorizer, - `percentage`- informs how much of tokens have been authrozied.
Add function that increases the authorization of the given staking provider for the given application by the given amount. Also add missing unit test for `Staking` service.
Add `stakingProvider` prop- the staking provider address is required to execute the `increaseAuthorization` transacion.
The minimum authorization amount is required only for the first time to increase app authorization. In other cases we can increase authorization amount with any value grater than 1.
Add `authorizationIncreased` action- should be dispatched when the dapp catches the `AuthorizationIncreased` event. This action will update the `authroizedStake` field for a given staking provider and given application.
Create a hook that subscribes to the `AuthorizationIncreased` and dispatch the `authorizationIncreased` to update application data for a given staking provider.
Preview uploaded to https://preview.dashboard.test.threshold.network/mas-integration/index.html. |
Add `staking` prrefix to hooks, file names and variables related to the staking applications beacause the `application` is to generic and we want to be more specific. The `application` can have different meanings and at first it sight is associated it with our Threshold web app as a whole instead of the staking application like random beacon or tbtc-v2.
Preview uploaded to https://preview.dashboard.test.threshold.network/mas-integration/index.html. |
We want to display percentage value in user-friendly format.
Preview uploaded to https://preview.dashboard.test.threshold.network/mas-integration/index.html. |
Preview uploaded to https://preview.dashboard.test.threshold.network/mas-integration/index.html. |
@@ -48,6 +49,8 @@ const Web3EventHandlerComponent = () => { | |||
useSubscribeToStakedEvent() | |||
useSubscribeToUnstakedEvent() | |||
useSubscribeToToppedUpEvent() | |||
useSubscribeToAuthorizationIncreasedEvent("tbtc") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you think we should use enums for these? And in other places in the d'app?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nevermind I see the type StakingAppName
return getThresholdLib(active ? library : undefined) | ||
}, [library, active]) | ||
useEffect(() => { | ||
if (active && library && account) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need a cleanup function here if the user disconnects?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const randomBeaconApp = useStakingAppDataByStakingProvider( | ||
"randomBeacon", | ||
stake.stakingProvider | ||
) | ||
|
||
// TODO: This will probably be fetched from contracts | ||
const appsAuthData = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it makes sense to store all of this information in the redux slice, rather than rebuilding these objects here. wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the docs https://redux.js.org/usage/deriving-data-selectors:
should keep the Redux state minimal, and derive additional values from that state whenever possible
I think it's better eg. calculate percentage in selector because less logic is needed to calculate this value, it depends on the total staked amount of a given stake- so we can keep them it sync with the staking data.
Regarding appsAuthData
and AppAuthDataProps
, I'm going to refactor this interface because it's problematic and gives a lot of unnecessary logic (see my PR #189). But I would like to address it in a separate PR.
@r-czajkowski I see this error when pulling changes. Is there a special setup I need?
|
export const useStakingAppDataByStakingProvider = ( | ||
appName: StakingAppName, | ||
stakingProvider: string | ||
) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have an interface defined somewhere that we could use for the return value of this func? and the other similar selector funcs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think we need this? When you use this hook or selector with useAppSelector
you will receive types automatically.
Set the default provider for the threshold lib when a user disconnects wallet.
I believe you have to update your dependencies and update the
|
The props in `StakeApplications` component has changed in #182- instead of the whole `StakeData` object we only pass `stakingProvider`.
Preview uploaded to https://preview.dashboard.test.threshold.network/mas-integration/index.html. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found 1 bug, see comment below
useEffect(() => { | ||
dispatch(stakingApplicationsSlice.actions.getSupportedApps({})) | ||
}, [dispatch]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a bug with getting minimum amount that is required to authorize for TBTC and Random Beacon. We fetch the data here in Staking/index.tsx
and it works fine when you go to the authorize apps page through the staking page:
Ther problem is when you refresh the page (or simply go straight to authorize apps url) and connect the wallet:
The minimum amount for TBTC and Random Beacon is not fetched properly as it shows 0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm gonna approve it since i haven't found any other issues.
Let's not forget about this bug: #178 (comment) and address it in a separate PR @r-czajkowski
Let's goo 🤙
`Token.TBTC` -> `Token.TBTCV1` `Token.TBTCV2` -> `Token.TBTC` In terms of threshold staking applications, we have only tbtc v2 app so I think from code's/T dapp's perspective is ok to use tbtc w/o v-2 because it's obvious that only tbtc-v2 works on threshold network. We only added the tbtc v1 token to get the token price in usd and calculate tvl. Ref: #178 (comment)
`Token.TBTC` -> `Token.TBTCV1` `Token.TBTCV2` -> `Token.TBTC` In terms of threshold staking applications, we have only tbtc v2 app so I think from code's/T dapp's perspective is ok to use tbtc w/o v-2 because it's obvious that only tbtc-v2 works on threshold network. We only added the tbtc v1 token to get the token price in usd and calculate tvl. Ref: threshold-network/token-dashboard#178 (comment)
Depends on: #165This PR integrates the data access layer added in #165 with MAS UI.
Main changes
Listener middleware
Add a listener middleware to redux store. Lets us define "listener" entries that contain an "effect" callback with additional logic, and a way to specify when that callback should run based on dispatched actions or state changes. It's helpful when we want to fetch data based on other fetched data and stored in the redux store. Eg. We want to fetch the application data for each owner stake- so we add a listener which will trigger an effect when the
setStakes
action will be dispatched. We could aslo create a one-shot listener by unconditionally callingunsubscribe()
in the body - the effect callback would run the first time the relevant action is seen, then immediately unsubscribe and never run again.Threshold-ts "lib"
EthereumConfig
type- add anaccount
field to be able to create a contract instance with a signer to signtransaction,
updateConfig
function toThreshold
class- to reinitiate services with updated config eg. we should call this function when a user connects to a wallet and an Ethereum provider changes,increaseAuthorization
function- to authorize a given staking provider for the given application by the given amount,Threshold React context
Threshold
lib to use the same instance with the same config in redux middleware and react context.Redux Store
staking-applications
slices to store the staking applications data such as authorization parameters for a given application and to store staking providers applications data,UI integration
Screenshots