feature: Get all Invalid Fills #1085
Conversation
There was a problem hiding this comment.
@dijanin-brat Looks good! I ended up commenting on some of the pre-existing code that's getting moved around; this isn't related to your work but probably makes sense to address it as part of this refactor.
| const enrichedDeposits = await Promise.all( | ||
| deposits.map(async (deposit: DepositWithBlock) => ({ | ||
| ...deposit, | ||
| quoteBlockNumber: await this.getBlockNumber(Number(deposit.quoteTimestamp)), | ||
| originChainId: this.chainId, | ||
| fromLiteChain: this.isOriginLiteChain(deposit), | ||
| toLiteChain: this.isDestinationLiteChain(deposit), | ||
| outputToken: isZeroAddress(deposit.outputToken) | ||
| ? this.getDestinationTokenForDeposit(deposit) | ||
| : deposit.outputToken, | ||
| })) | ||
| ); |
There was a problem hiding this comment.
@dijanin-brat ooc, should this be 1:1 with the EVM implementation?
There was a problem hiding this comment.
They work with 2 different types of data (EVM has Log and SVM has DepositWithBlock). In SVM we are just adding values for few more fields, in EVM we are creating whole new structure. I guess this can be refactored in a way that both EVM and SVM use the same functiom/method for this. Do you think that should be done?
There was a problem hiding this comment.
For follow-up after this PR, yeah. I'm pretty sure they should using the same underlying types, since DepositWithBlock is the product of processing a Log type. So the fact that these are different hints at some unexpected asymmetry between the implementations.
There was a problem hiding this comment.
@melisaguevara @bmzig You're both strong on the SVMSpokePoolClient - possible to take a pass over this file?
| // First do all synchronous operations | ||
| deposits = events.map((event) => { | ||
| const deposit = { | ||
| ...spreadEventWithBlockNumber(event), |
There was a problem hiding this comment.
This PR looks fine to me. I'd prefer if we can merge this after we do the address migration though. Once we do that, you'd need to manually cast the address args here (like depositor, recipient, etc.) to an Address type.
| if (isZeroAddress(deposit.outputToken)) { | ||
| deposit.outputToken = this.getDestinationTokenForDeposit(deposit); | ||
| } |
There was a problem hiding this comment.
fwiw I think this needs to be re-worked following the Address class updates that were merged on Friday.
Get all invalid fills