-
Notifications
You must be signed in to change notification settings - Fork 44
Transaction batching + new History endpoint #843
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
Transaction batching + new History endpoint #843
Conversation
|
📦 build.zip [updated at Oct 22, 10:48:00 AM UTC] |
e5207a1 to
12540c3
Compare
| action: AddressAction | null; | ||
| }; | ||
|
|
||
| async function convertToNewInterpretation( |
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.
Temporary function until backend support new data scheme in the new ZPI endpoint
|
|
||
| invariant(testAddress, 'TEST_WALLET_ADDRESS Env var not found'); | ||
|
|
||
| const cancelTxSample = { |
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 to recreate this test with the new scheme?
b7a7688 to
2e2a4f3
Compare
|
Do not forget to change backend_env back before release |
3cb877f to
d1d33e4
Compare
| try { | ||
| const typedData = JSON.parse(data); | ||
| const typedData = JSON.parse(data) as TypedData; | ||
| // Backend expect chainId to be a 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.
| // Backend expect chainId to be a string | |
| // Backend expects chainId to be a string |
| // TODO: remove when backend fixed | ||
| typedData.domain.chainId = typedData.domain.chainId | ||
| ? String(typedData.domain.chainId) | ||
| : undefined; |
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.
- remove?
- this is safer:
| // TODO: remove when backend fixed | |
| typedData.domain.chainId = typedData.domain.chainId | |
| ? String(typedData.domain.chainId) | |
| : undefined; | |
| // TODO: remove when backend fixed | |
| if (typedData.domain.chainId) { | |
| typedData.domain.chainId = String(typedData.domain.chainId) | |
| } |
| 'actIndex should be a number or be empty' | ||
| ); | ||
| const actIndex = act_index ? Number(act_index) : undefined; | ||
| const addressAction = state.addressAction as AddressAction | undefined; |
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.
state is undefined on direct navigation, leading to crash
| if (typeof data === 'string') { | ||
| try { | ||
| const typedData = JSON.parse(data); | ||
| const typedData = JSON.parse(data) as TypedData; |
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.
The next line does the same casting. Is this necessary?
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.
rm?
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.
It's already in the main (🫠), so let's remove it at least here
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.
rm?
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.
rm?
| if ( | ||
| allowanceQuantityCommon == null || | ||
| addressAction?.acts?.length !== 1 || | ||
| addressAction.acts[0].content?.approvals?.length !== 1 | ||
| ) { | ||
| return addressAction; | ||
| } | ||
| return produce(addressAction, (draft) => { | ||
| if (draft.acts?.[0].content?.approvals?.[0]) { | ||
| if (draft.acts[0].content.approvals[0].amount) { | ||
| draft.acts[0].content.approvals[0].amount.quantity = | ||
| allowanceQuantityCommon; | ||
| } else { | ||
| draft.acts[0].content.approvals[0].amount = { | ||
| quantity: allowanceQuantityCommon, | ||
| value: null, | ||
| usdValue: null, | ||
| currency: '', | ||
| }; | ||
| } | ||
| if (customAllowanceQuantityBase != null) { | ||
| draft.acts[0].content.approvals[0].unlimited = isUnlimitedApproval( | ||
| customAllowanceQuantityBase | ||
| ); | ||
| } | ||
| } | ||
| }); |
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.
Maybe this logic is better placed outside of UI?
Perhaps in /src/modules/ethereum/transactions/appovals.ts? Or nearby:)
| <VStack gap={0}> | ||
| <UIText kind="headline/h3">{collection.name}</UIText> | ||
| </VStack> |
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.
is VStack needed?
| ); | ||
|
|
||
| // Special request for analytics purposes. | ||
| const { data: inputFungibleUsdInfo } = useAssetFullInfo( |
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 { data: inputFungibleUsdInfo } = useAssetFullInfo( | |
| const { data: inputFungibleUsdInfoForAnalytics } = useAssetFullInfo( |
| </VStack> | ||
| {actions.length && (isLoading || hasMore) ? ( | ||
| <SurfaceList | ||
| style={{ paddingBlockStart: 6, paddingBlockEnd: 6 }} |
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.
?
| style={{ paddingBlockStart: 6, paddingBlockEnd: 6 }} | |
| style={{ paddingBlock: 6 }} |
src/ui/pages/History/History.tsx
Outdated
| const backendHashes = new Set(backend.map((tx) => tx.transaction.hash)); | ||
| const backendHashes = new Set( | ||
| backend.map( | ||
| (tx) => tx.transaction?.hash || tx.acts?.at(0)?.transaction.hash |
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 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.
Probably needs a comment. For now we can generate locally only txs with one hash, so it should be enough to compare the hash of the first act. But may be it is actually more reliable to go through all acts there
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.
Updated!
| /> | ||
| </Button> | ||
| <KeyboardShortcut | ||
| combination="cmd+r" |
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.
Let's maybe mention this shortcut in the refresh button's title?
src/ui/pages/SendForm/SendForm.tsx
Outdated
| }; | ||
|
|
||
| // Special request for analytics purposes. | ||
| const { data: inputFungibleUsdInfo } = useAssetFullInfo( |
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 { data: inputFungibleUsdInfo } = useAssetFullInfo( | |
| const { data: inputFungibleUsdInfoForAnalytics } = useAssetFullInfo( |
| // const functionName = useMemo( | ||
| // () => | ||
| // interpretation ? getInterpretationFunctionName(interpretation) : null, | ||
| // [interpretation] | ||
| // ); |
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.
rm comments in this file? Is this the final version?
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.
Yes, I will remove comments!
src/ui/pages/SwapForm/SwapForm.tsx
Outdated
| useEffect(() => setAllowanceBase(null), [inputAmount, inputFungibleId]); | ||
|
|
||
| // Special request for analytics purposes. | ||
| const { data: inputFungibleUsdInfo } = useAssetFullInfo( |
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 { data: inputFungibleUsdInfo } = useAssetFullInfo( | |
| const { data: inputFungibleUsdInfoForAnaytics } = useAssetFullInfo( |
src/ui/shared/requests/interpret.ts
Outdated
| { source }: BackendSourceParams | ||
| ): Promise<InterpretResponse | null> { | ||
| return Promise.race([ | ||
| rejectAfterDelay(10000, 'interpret transaction'), |
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.
Why do we need a reject here? Http request should get rejected on its own?
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.
Correct! That is the legacy from the socket api. Will remove!
src/ui/shared/requests/interpret.ts
Outdated
| { source }: BackendSourceParams | ||
| ): Promise<SignatureInterpretResponse> { | ||
| return Promise.race([ | ||
| rejectAfterDelay(10000, 'interpret signature'), |
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 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.
See comments
In this PR new history endpoint is being introduced.
What was changed:
In the nearest future transaction simulation will move to the new scheme, so all the places where we work with simulations were updated too
Also with this update we are moving away from using asset implementations as much as possible.
For now we still need them for Send Form and Edit Approve Form, so there were added some extra logic to keep base quantity instead of common where it is needed.
Partially, simulations components were moved to networkConfig as a prop instead of fetching networks.
TODO
Questions to discuss