[#522] chore: refactor pending transaction #523
Conversation
jdyczka
requested review from
MSzalowski,
Sworzen1,
JanJaroszczak and
kickloop
as code owners
jdyczka
force-pushed
the
transaction-state
branch
from
0f0d73c to
48a585e
Compare
|
Provide PR description |
| @@ -324,14 +302,14 @@ export const DashboardCards = () => { | |||
| <Box | |||
| sx={{ | |||
| columnGap: 3, | |||
| display: "grid", | |||
| display: 'grid', | |||
| gridTemplateColumns: | |||
| screenWidth < 1280 | |||
There was a problem hiding this comment.
If refactor of this component already happened - it would be great to also get rid of this nested ternaries
| Box, CircularProgress, Tab, Tabs, styled, | ||
| } from "@mui/material"; | ||
| import { useLocation } from "react-router-dom"; | ||
| Box, CircularProgress, Tab, Tabs, styled |
There was a problem hiding this comment.
Revert this object-curly-newline is faulty set rule and will be removed
| { | ||
| proposalId: getFullGovActionId( | ||
| onClick={() => | ||
| (onDashboard && |
There was a problem hiding this comment.
get rid of nested ternaries
There was a problem hiding this comment.
Probably @JanJaroszczak will be modifying this file, so it I will leave it as it to avoid conflicts
| const keys = multiasset.keys(); // policy Ids of thee multiasset | ||
| const N = keys.len(); | ||
|
|
||
| for (let i = 0; i < N; i++) { |
There was a problem hiding this comment.
Why not just iterate through keys? What is the reason for using nested for loops with having the builtin methods that does the job for us? 🤔
| resourceId?: never; | ||
| }; | ||
|
|
||
| export type TransactionStateWithResource = { |
There was a problem hiding this comment.
They are exactly the same except type which might be passed as generic
There was a problem hiding this comment.
resourceId is required in one but not the other
| export type TransactionStateWithResource = { | ||
| type: TransactionTypeWithResource; | ||
| transactionHash: string; | ||
| time: Date; |
There was a problem hiding this comment.
If time comes from an API it couldn't be of type Date, as JSON only allows primitive types
There was a problem hiding this comment.
its point of time which describes when transaction was confirmed, only for internal service/hook connected with pending transactions
| let count = 0; | ||
| let isDBSyncUpdated = false; | ||
| while (!isDBSyncUpdated && count < DB_SYNC_MAX_ATTEMPTS) { | ||
| count++; |
There was a problem hiding this comment.
We should avoid ++ as it might cause hard-to-debug problems on production
There was a problem hiding this comment.
what do you recomend for this solution ?, I mean insted of while interation
There was a problem hiding this comment.
isn't it confusing only as art of a bigger expression?
| }; | ||
|
|
||
| const isTransactionExpired = (time: Date): boolean => | ||
| new Date().getTime() - time.getTime() > TIME_TO_EXPIRE_TRANSACTION; |
There was a problem hiding this comment.
Why not just Date.now()? It is ~2 times faster in execution than new Date().getTime() giving the same result
| pendingTransaction.registerAsDrep || | ||
| pendingTransaction.registerAsSoleVoter || | ||
| pendingTransaction.retireAsDrep || | ||
| pendingTransaction.retireAsSoleVoter, | ||
| ], | ||
| enabled: !!dRepID, | ||
| queryFn: async () => await getVoterInfo(dRepID), |
There was a problem hiding this comment.
By the way - you could also remove async and await here as they are redundant
Co-authored-by: Bartłomiej Sworzeń <b.sworzen@gmail.com>
jdyczka
force-pushed
the
transaction-state
branch
2 times, most recently
from
210d91c to
66ebbc6
Compare
jdyczka
force-pushed
the
transaction-state
branch
from
66ebbc6 to
467460f
Compare
List of changes
Checklist