Feat: operations refactoring #2772
Open
+8,074
−4,040
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
# Conflicts: # src/renderer/features/wallet-select/model/__tests__/wallet-select-model.test.ts # src/renderer/features/wallets/WalletSelect/ui/WalletSelect.tsx # src/renderer/widgets/WalletDetails/model/__tests__/wallet-provider-model.test.ts
# Conflicts: # src/renderer/features/wallet-details/model/wc-details-model.ts
# Conflicts: # src/renderer/app/index.tsx
# Conflicts: # src/renderer/widgets/CreateWallet/ui/MultisigWallet/components/Signatory.tsx
# Conflicts: # src/renderer/features/proxy-remove-pure/model/remove-pure-proxy-model.ts # src/renderer/features/proxy-remove/model/remove-proxy-model.ts
…into feat/flexible-multisig
feat: add bond details feat: add wallet info from tx
Jest Unit tests resultsDuration: 45.738 seconds
Outcome: Passed | Total Tests: 750 | Passed: 746 | Failed: 0
|
johnthecat
reviewed
Dec 5, 2024
| }; | ||
|
|
||
| export const operationDetailsSlot = createSlot<SlotProps>(); | ||
| export const operationTitleSlot = createSlot<SlotProps>(); |
There was a problem hiding this comment.
I think slot should be defined in same file, where it's used in the first place
# Conflicts: # src/renderer/features/fellowship-members/components/MembersModal.tsx
# Conflicts: # src/renderer/widgets/AssetTransactionModal/ui/AssetTransactionModal.tsx
…et/omni-enterprise into feat/operations-refactoring
Asmadek
changed the title
…et/omni-enterprise into feat/operations-refactoring
johnthecat
reviewed
Dec 9, 2024
| name: 'governance/operation-details', | ||
| }); | ||
|
|
||
| governanceOperationDetailFeature.inject(multisigOperationsFeature.slots.operationDetails, { |
There was a problem hiding this comment.
For simplicity you can separate injections by transaction type, one for each
tuul-wq
reviewed
Dec 10, 2024
| blockCreated?: number, | ||
| explorers?: Explorer[], | ||
| ): string | undefined => { | ||
| if (!callHash || !indexCreated || !blockCreated || !explorers) return; |
There was a problem hiding this comment.
This function cannot execute if at least one argument is absent, looks pretty strange to me.
Let's check data existence before calling the function
| signatories: Signatory[], | ||
| chainId: ChainId, | ||
| ): Account[] => { | ||
| const walletsMap = new Map(wallets.map((wallet) => [wallet.id, wallet])); |
There was a problem hiding this comment.
Could use dictionary method instead
dictionary(wallets, 'id');| ): Account[] => { | ||
| const walletsMap = new Map(wallets.map((wallet) => [wallet.id, wallet])); | ||
|
|
||
| return signatories.reduce((acc: Account[], signatory) => { |
| operations?: MultisigTransaction[]; | ||
| }; | ||
|
|
||
| export const OperationList = memo(({ operations }: Props) => { |
There was a problem hiding this comment.
List will be a new reference every time, there cannot be any memoization
| <TransactionTitle className="flex-1 overflow-hidden" tx={tx.transaction} /> | ||
|
|
||
| {asset && amount && ( | ||
| <div className="w-[160px]"> |
Comment on lines
+92
to
+102
| if (!transaction) { | ||
| return ( | ||
| <> | ||
| <TransactionTitle className="flex-1 overflow-hidden" tx={transaction} /> | ||
|
|
||
| <ChainTitle chainId={operation.chainId} className="w-[114px]" /> | ||
| </> | ||
| ); | ||
| } | ||
|
|
||
| return null; |
There was a problem hiding this comment.
Suggested change
| if (!transaction) { | |
| return ( | |
| <> | |
| <TransactionTitle className="flex-1 overflow-hidden" tx={transaction} /> | |
| <ChainTitle chainId={operation.chainId} className="w-[114px]" /> | |
| </> | |
| ); | |
| } | |
| return null; | |
| if (transaction) return null; | |
| return ( | |
| <> | |
| <TransactionTitle className="flex-1 overflow-hidden" tx={transaction} /> | |
| <ChainTitle chainId={operation.chainId} className="w-[114px]" /> | |
| </> | |
| ); |
| ); | ||
| } | ||
|
|
||
| return <>{result.map((e) => e)}</>; |
There was a problem hiding this comment.
Any point to use array with a single element?
Comment on lines
+36
to
+37
| const sender = operationDetailsUtils.getSender(operation); | ||
| const destinationChain = operationDetailsUtils.getDestinationChain(operation); |
There was a problem hiding this comment.
Let's move these line up to const destination for better group composition
| @@ -70,7 +77,7 @@ export const OperationFullInfo = ({ tx, account }: Props) => { | |||
| )} | |||
| </div> | |||
|
|
|||
| <OperationCardDetails tx={tx} account={account} extendedChain={extendedChain} /> | |||
| <div className="flex w-full flex-col gap-y-1">{operationDetails}</div> | |||
|
|
||
| export const registerFeatures = (features: Feature<unknown>[]) => { | ||
| // Basically groupBy | ||
| const domains = features.reduce<Record<string, Feature<unknown>[]>>((acc, feature) => { |
johnthecat
reviewed
Dec 13, 2024
|
|
||
| const delegationTarget = operationDetailsUtils.getDelegationTarget(operation); | ||
| const delegationTracks = operationDetailsUtils.getDelegationTracks(operation); | ||
| const delegationVotes = operationDetailsUtils.getDelegationVotes(operation); |
There was a problem hiding this comment.
I think we can keep it as is for now, but main idea is to fully decouple views and logic, so parsing of particular transaction info should be bounded to related context (domain or feature). In this case parsing of delegation details should be somewhere in governance services, not in operations utils.
Add TODO please!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
feat: add transfer details
feat: add bond details
feat: add wallet info from tx