Blacklisted accounts #4683
Blacklisted accounts #4683
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| setAddresses(result.map((key: any) => encodeAddress(key.args[0]))) | ||
| }) | ||
| const accountsOptedOut = useFirstObservableValue(() => { | ||
| return api?.query.referendum?.accountsOptedOut.keys() |
There was a problem hiding this comment.
I think the skip should be kept because the chain shouldn't be queried every time useStakingAccountsLocks is called:
| return api?.query.referendum?.accountsOptedOut.keys() | |
| return skip ? undefined : api?.query.referendum?.accountsOptedOut.keys() |
| useEffect(() => { | ||
| if (!isVoting) return | ||
| if (!isVoting || !votingOptOutAccounts) return |
There was a problem hiding this comment.
So unless I'm missing something, I don't think this is needed:
| if (!isVoting || !votingOptOutAccounts) return | |
| if (!isVoting) return |
| const mockDefaultBalance = { | ||
| locked: BN_ZERO, | ||
| recoverable: BN_ZERO, | ||
| transferable: BN_ZERO, | ||
| locks: [], | ||
| vestingTotal: BN_ZERO, | ||
| vestedClaimable: BN_ZERO, | ||
| vestedBalance: BN_ZERO, | ||
| vestingLocked: BN_ZERO, | ||
| vesting: [], | ||
| total: BN_THOUSAND, | ||
| } | ||
|
|
||
| const useMyBalances: AddressToBalanceMap = { | ||
| [allAccounts[0]]: { | ||
| ...mockDefaultBalance, | ||
| total: new BN(200_000_000_000), | ||
| }, | ||
| [allAccounts[1]]: { | ||
| ...mockDefaultBalance, | ||
| total: new BN(200_000_000_000), | ||
| }, | ||
| [allAccounts[2]]: { | ||
| ...mockDefaultBalance, | ||
| total: new BN(200_000_000_000), | ||
| }, | ||
| [allAccounts[3]]: { | ||
| ...mockDefaultBalance, | ||
| total: new BN(200_000_000_000), | ||
| }, | ||
| } |
There was a problem hiding this comment.
I don't think this is needed:
| const mockDefaultBalance = { | |
| locked: BN_ZERO, | |
| recoverable: BN_ZERO, | |
| transferable: BN_ZERO, | |
| locks: [], | |
| vestingTotal: BN_ZERO, | |
| vestedClaimable: BN_ZERO, | |
| vestedBalance: BN_ZERO, | |
| vestingLocked: BN_ZERO, | |
| vesting: [], | |
| total: BN_THOUSAND, | |
| } | |
| const useMyBalances: AddressToBalanceMap = { | |
| [allAccounts[0]]: { | |
| ...mockDefaultBalance, | |
| total: new BN(200_000_000_000), | |
| }, | |
| [allAccounts[1]]: { | |
| ...mockDefaultBalance, | |
| total: new BN(200_000_000_000), | |
| }, | |
| [allAccounts[2]]: { | |
| ...mockDefaultBalance, | |
| total: new BN(200_000_000_000), | |
| }, | |
| [allAccounts[3]]: { | |
| ...mockDefaultBalance, | |
| total: new BN(200_000_000_000), | |
| }, | |
| } |
There was a problem hiding this comment.
I don't think this is needed:
After removing this and implementing other changes this error shows up
When writing the storybook test i came across the error earlier on and i figured mocking the locks that way would be the fix for it which was i used this. In toBalance the lockdetails are been used so i thought the storybook test needed such mock data
| const allAccounts = [ | ||
| 'j4Rc8VUXGYAx7FNbVZBFU72rQw3GaCuG2AkrUQWnWTh5SpemP', | ||
| 'j4RjraznxDKae1aGL2L2xzXPSf8qCjFbjuw9sPWkoiy1UqWCa', | ||
| 'j4RbTjvPyaufVVoxVGk5vEKHma1k7j5ZAQCaAL9qMKQWKAswW', | ||
| 'j4Rh1cHtZFAQYGh7Y8RZwoXbkAPtZN46FmuYpKNiR3P2Dc2oz', | ||
| ] |
There was a problem hiding this comment.
It's possible to use the account addresses directly, but the extensions are also supposed to provide a name so it's clearer IMO to just work on members (they all have a controller account which will be used in the mock:
| const allAccounts = [ | |
| 'j4Rc8VUXGYAx7FNbVZBFU72rQw3GaCuG2AkrUQWnWTh5SpemP', | |
| 'j4RjraznxDKae1aGL2L2xzXPSf8qCjFbjuw9sPWkoiy1UqWCa', | |
| 'j4RbTjvPyaufVVoxVGk5vEKHma1k7j5ZAQCaAL9qMKQWKAswW', | |
| 'j4Rh1cHtZFAQYGh7Y8RZwoXbkAPtZN46FmuYpKNiR3P2Dc2oz', | |
| ] | |
| const allAccounts = [ | |
| { member: member('alice'), balance: 20 }, | |
| { member: member('bob'), balance: 20 }, | |
| { member: member('charlie'), balance: 20 }, | |
| { member: member('alice'), balance: 20 }, | |
| ] |
Finally here 20 should mean 20JOY not 20HAPI
packages/ui/src/app/pages/Election/BlacklistedAccounts/BlacklistedAccounts.stories.tsx
Show resolved
Hide resolved
| }, | ||
| referendum: { | ||
| accountsOptedOut: { | ||
| keys: [...allAccounts, ...allAccounts, ...allAccounts, ...allAccounts, ...allAccounts], |
There was a problem hiding this comment.
The list of addresses should be: allAccounts.map(({ member }) => member.controllerAccount).
So to trigger the pagination:
| keys: [...allAccounts, ...allAccounts, ...allAccounts, ...allAccounts, ...allAccounts], | |
| keys: Array.from({ length: 5 }) | |
| .fill(allAccounts.map(({ member }) => member.controllerAccount)) | |
| .flat(), |
| const Template: Story = () => { | ||
| return ( | ||
| <BalancesContext.Provider value={useMyBalances}> | ||
| <BlacklistedAccounts /> | ||
| </BalancesContext.Provider> | ||
| ) | ||
| } | ||
|
|
||
| export const Default = Template.bind({}) |
There was a problem hiding this comment.
So no need for an additional provider:
| const Template: Story = () => { | |
| return ( | |
| <BalancesContext.Provider value={useMyBalances}> | |
| <BlacklistedAccounts /> | |
| </BalancesContext.Provider> | |
| ) | |
| } | |
| export const Default = Template.bind({}) | |
| export const Default: Story = {} |
| }, | ||
| } | ||
| export default { | ||
| title: 'Pages/Blacklisted Accounts', |
There was a problem hiding this comment.
Finally this should be under "Election":
| title: 'Pages/Blacklisted Accounts', | |
| title: 'Pages/Election/Blacklisted Accounts', |
| @@ -242,7 +242,7 @@ export const proposalsPagesChain = ( | |||
| nextRewardPayments, | |||
| stage: { stage: { isIdle: true }, changedAt: 123 }, | |||
| }, | |||
| referendum: { stage: {} }, | |||
| referendum: { stage: {}, accountsOptedOut: { keys: [] } }, | |||
There was a problem hiding this comment.
So with the skip argument, this is not necessary
| referendum: { stage: {}, accountsOptedOut: { keys: [] } }, |
| // const withComputed = (data: Record<any, any>) => Object.defineProperties(data, { | ||
| // unwrap: { value: () => data }, | ||
| // isSome: { value: Object.keys(data).length > 0 }, | ||
| // keys: { value: () => of(Object.keys(data).map(entry => ({ args: [entry] }))) }, | ||
| // }) |
There was a problem hiding this comment.
Please remove the commented code on this file.
You're right the value can't be returned from the asChainData, instead something similar could be done on asApiMethod but it's not urgent right now...
There was a problem hiding this comment.
Also it seems the balance encoding broke some tests. Please have a quick look. If there's no quick solution I'll fix it tomorrow.
Finally even if it works on storybook, this will require some actual QA before merging it. @ivanturlakov could you please create a bunch of accounts on Atlas dev transfer them some funds and Opt out of voting with them please ?
| @@ -1,5 +1,6 @@ | |||
| import { createType } from '@joystream/types' | |||
| import { mapValues } from 'lodash' | |||
| // import { of } from 'rxjs' | |||
There was a problem hiding this comment.
| // import { of } from 'rxjs' |
| const TotalBalance = ({ address, handleTotalBalance }: TotalBalanceProp) => { | ||
| const balance = useBalance(address) | ||
|
|
||
| const setTotalBalance = (number: BN) => { | ||
| handleTotalBalance(number) | ||
| } | ||
|
|
||
| useEffect(() => { | ||
| if (balance) setTotalBalance(balance.total) | ||
| }, [balance]) | ||
|
|
||
| return <></> | ||
| } |
There was a problem hiding this comment.
This is a pattern to avoid. Instead please add a useBalances hook in packages/ui/src/accounts/hooks/useBalance.ts:
export const useBalances = (addresses: Address[]): Map<string, Balances | undefined> => {
const { api } = useApi()
const allMyBalances = useMyBalances()
const [definedBalances, setDefinedBalances] = useState(new Map<string, Balances>())
useEffect(() => {
addresses.forEach((address) => {
if (definedBalances.get(address)) return
addressToBalances(api, allMyBalances, address).then((balances) => {
if (balances) setDefinedBalances((entries) => new Map([...entries.entries(), [address, balances]]))
})
})
}, [api, allMyBalances])
return useMemo(() => new Map(addresses.map((address) => [address, definedBalances.get(address)])), [definedBalances])
}
const addressToBalances = async (
api: Api | undefined,
allMyBalances: AddressToBalanceMap | undefined,
address: string
): Promise<Balances | undefined> => {
if (!allMyBalances) return
const myBalances = allMyBalances[address]
if (myBalances) return myBalances
if (!api) return
return toBalances(await firstValueFrom(api.derive.balances.all(address)))
}That was trickier to write than I thought and I haven't tested it yet. But IMO it's better than having fake components to loop over hooks.
Also with this hook the component should be simplified a bit.
@thesan sure, what atlas instance should I use? |
@ivanturlakov I meant the Atlas dev network. However it seems that there is an opted out account already. So I guess nothing more is needed. Sorry I should have checked yesterday but I was convinced there was no opted out account on mainnet ATM. @vrrayz please check: https://dao-git-fork-vrrayz-blacklisted-accounts-joystream.vercel.app/#/election/blacklisted-accounts there is a bug with the total counter, my guess is that it comes from: #4683 (comment) I'm surprised it didn't show on storybook (I guess the full app is triggering a lot more re-renders). |
| export const BlacklistedAccount = ({ address }: Prop) => { | ||
| const balance = useBalance(address) |
There was a problem hiding this comment.
For most users useBalance will call derive.balances.all. RPC node queries are not cached and they should be reduced as much as possible. Since useAddresses is called on this page, this component shouldn't call useBalance. Instead:
| export const BlacklistedAccount = ({ address }: Prop) => { | |
| const balance = useBalance(address) | |
| export const BlacklistedAccount = ({ address, balance }: Prop) => { |
| const ACCOUNTS_PER_PAGE = 18 | ||
| const [page, setPage] = useState(1) | ||
| const votingOptOutAccounts = useVotingOptOutAccounts() | ||
| const [paginatedAccounts, setPaginatedAccounts] = useState<string[] | undefined>() |
There was a problem hiding this comment.
As a result useBalances should be called on this component instead. Also I think the sum can be done here (no need for TotalBalance), and the paginated accounts should be { address: string, balance: BN | undefined }:
| const [paginatedAccounts, setPaginatedAccounts] = useState<string[] | undefined>() | |
| const balances = useBalances(addresses) | |
| const totalBalance = useMemo( | |
| () => balances.values().reduce((prev, value) => value?.total.add(prev) ?? prev, BN_ZERO), | |
| [balances] | |
| ) | |
| const paginatedAccounts = useMemo( | |
| () => | |
| votingOptOutAccounts | |
| .slice((page - 1) * ACCOUNTS_PER_PAGE, page * ACCOUNTS_PER_PAGE) | |
| .map((address) => ({ address, balance: balance.get(address)?.total })) | |
| [votingOptOutAccounts, page, balances]) | |
| ) |
|
|
||
| if (myBalances) { | ||
| return myBalances | ||
| } | ||
|
|
||
| return balances ? toBalances(balances) : null | ||
| } | ||
| export const useBalances = (addresses: Address[]): Map<string, Balances | undefined> => { |
There was a problem hiding this comment.
Small detail but please skip a line here:
| export const useBalances = (addresses: Address[]): Map<string, Balances | undefined> => { | |
| export const useBalances = (addresses: Address[]): Map<string, Balances | undefined> => { |
| const [definedBalances, setDefinedBalances] = useState(new Map<string, Balances>()) | ||
| useEffect(() => { | ||
| addresses.forEach((address) => { | ||
| if (definedBalances.get(address)) return |
There was a problem hiding this comment.
Even smaller detail (and I know it's something I wrote) but:
| if (definedBalances.get(address)) return | |
| if (definedBalances.has(address)) return |
Fix for #4138