feat: fetch token deposits from event logs #2168
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
|
||
| expect(result).toHaveLength(1) | ||
| expect(result).toEqual( | ||
| expect.arrayContaining([ |
There was a problem hiding this comment.
How are the results significantly different from fetchDepositsFromSubgraph test? I'd assume fetching deposits through event logs, with an additional sender condition, would be a subset of overall deposits being fetched via subgraph, but there seems to be no intersection. 🤔
There was a problem hiding this comment.
fetchDepositsFromSubgraph only tests for ETH deposits so this token deposits tests will be all different
in fact, we need to add tests for token deposits fetches to the subgraph test
| parentChainId: 1, | ||
| pageSize: 100, | ||
| parentGatewayAddresses: [ | ||
| '0xa3A7B6F88361F48403514059F1F16C8E78d60EeC', // Parent Standard Gateway |
There was a problem hiding this comment.
We shouldn't redefine the addresses here. Instead, should reuse them from an existing source. Eg. getArbitrumNetwork(42161).tokenBridge.parentErc20Gateway if that works.
| pageSize: 100 | ||
| childChainId: 42161, | ||
| parentChainId: 1, | ||
| pageSize: 100, |
There was a problem hiding this comment.
Is pageSize required in the API?
There was a problem hiding this comment.
this file is reused between subgraph and event logs tests
| } | ||
|
|
||
| /** | ||
| * Fetches initiated token withdrawals from event logs in range of [fromBlock, toBlock]. |
There was a problem hiding this comment.
| * Fetches initiated token withdrawals from event logs in range of [fromBlock, toBlock]. | |
| * Fetches initiated token deposits from event logs in range of [fromBlock, toBlock]. |
There was a problem hiding this comment.
Pls update the remainder of comments to have deposits instead of withdrawals~~
| * @param query.parentGatewayAddresses L2 gateway addresses to use | ||
| */ | ||
| export async function fetchTokenDepositsFromEventLogs({ | ||
| sender, |
There was a problem hiding this comment.
We must decide between sender vs fromAddress, and receiver vs toAddress in this file and stick to one.
There was a problem hiding this comment.
getDepositEvents is following the format of getWithdrawalEvents in arbitrum sdk's erc20Bridger
we'll move this function to arbitrum sdk later so i'll keep fromAddress and toAddress for it
|
|
||
| parentGatewayAddresses.forEach(gatewayAddress => { | ||
| // funds sent by this address | ||
| if (sender) { |
There was a problem hiding this comment.
Are we okay with returning empty data if no sender or receiver is passed?
getDepositEvents supports sender and receiver being null.
There was a problem hiding this comment.
This is how we're handling it at the moment with fetchTokenWithdrawalsFromEventLogs so following that format and i think that's fine
| L1ERC20Gateway__factory, | ||
| contract => | ||
| contract.filters.DepositInitiated( | ||
| null, // parentToken |
There was a problem hiding this comment.
Can't we pass parentTokenAddress here?
There was a problem hiding this comment.
fairly sure it's a typechain typing issue
we are doing the same with withdrawals
| childChainId: 42161, | ||
| parentChainId: 1, | ||
| parentGatewayAddresses: [ | ||
| getArbitrumNetwork(42161).tokenBridge!.parentErc20Gateway, |
There was a problem hiding this comment.
nit: we could have getArbitrumNetwork(42161) as a variable
| ) { | ||
| return [...new Map(events.map(item => [item.txHash, item])).values()] | ||
| } | ||
| import { dedupeEvents } from '../deposits/helpers' |
There was a problem hiding this comment.
Withdrawals shouldn't really import from deposits helpers, maybe we can move it to /TransactionHistory/helpers instead?
Closes FS-1071