Skip to content
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

feat: identify eth deposits #405

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

amateima
Copy link
Collaborator

No description provided.

Copy link

linear bot commented May 13, 2024

@amateima amateima requested a review from dohaki May 13, 2024 11:59
Copy link
Contributor

@dohaki dohaki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a few questions and remarks

Comment on lines 1 to 37
import { BigNumber, Event } from "ethers";

export interface WethDepositEvent extends Event {
args: [string, BigNumber] & {
dst: string;
wad: BigNumber;
};
}

export interface WethDepositEventOptimism extends Event {
args: [string, BigNumber] & {
dst: string;
wad: BigNumber;
};
}

export interface WethDepositEventLinea extends Event {
args: [string, BigNumber] & {
dst: string;
wad: BigNumber;
};
}

export interface WethDepositEventBase extends Event {
args: [string, BigNumber] & {
dst: string;
wad: BigNumber;
};
}

export interface WethTransfetEventArbitrum extends Event {
args: [string, string, BigNumber] & {
from: string;
to: string;
value: BigNumber;
};
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason why we need duplicated events for different chains? AFAICT only on Arbitrum the respective event is different. But can't we just reuse the Deposit event signature for every other chain? This would also decrease the number of else if cases in the code

};
}

export interface WethTransfetEventArbitrum extends Event {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
export interface WethTransfetEventArbitrum extends Event {
export interface WethTransferEventArbitrum extends Event {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need a different ABI file for every chain if they are the same, no?

Copy link
Collaborator Author

@amateima amateima May 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately the ABIs are not the same and I didn't want to spend time putting together a common ABI file

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm okay, got you.. Maybe we could also just reduce the ABI to the minimal interfaces with need / care about, i.e.

  {
    "anonymous": false,
    "inputs": [
      {
        "indexed": true,
        "name": "dst",
        "type": "address"
      },
      {
        "indexed": false,
        "name": "wad",
        "type": "uint256"
      }
    ],
    "name": "Deposit",
    "type": "event"
  },

Comment on lines +291 to +295
return (
e.logIndex < depositEvent.logIndex &&
typedEvent.args.value.eq(depositEvent.args.inputAmount) &&
spokePoolAddresses.includes(typedEvent.args.to)
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we need to also add the check

typedEvent.args.from === ZERO_ADDRESS

? So that we don't consider a normal WETH transfer but rather a "deposit", i.e. mint.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spokePoolAddresses.includes(typedEvent.args.to) checks if the recipient of WETH is the SpokePool contract. Do you think it's necessary to also check from address? I cannot see a case when someone sends WETH intentionally to the SpokePool followed by a deposit

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, you are right. Should be very unlikely

@amateima amateima force-pushed the amatei/acx-2136-identify-eth-weth-swap branch from e653e79 to 32954f0 Compare May 13, 2024 13:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants