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.

Sign up for GitHub

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

Jump to bottom

chore: post contracts package cleanup #7687

Merged
merged 26 commits into from
Sep 13, 2024
Merged

chore: post contracts package cleanup #7687

merged 26 commits into from
Sep 13, 2024

Conversation

woodenfurniture
Copy link
Member

@woodenfurniture woodenfurniture commented Sep 4, 2024 •
edited
Loading

Description

Consolidates contract addresses and ABIs into the contacts package.

  • extra defined abis and contract address constants in unchained-client
  • addresses and abis in the investor packages
  • also a handful in the thorchain swapper related to univ3 longtail

Issue (if applicable)

closes #7681

Risk

High Risk PRs Require 2 approvals

High risk as this affects orchestration of user funds.

What protocols, transaction types, wallets or contract interactions might be affected by this PR?

Affects all EVM + THORChain protocols across all features aside from sending and receiving.

Testing

  • Swaps, focusing on thorchain and evms (but also test utxos and cosmossdk)
  • ERC20 approvals
  • All features involving EVMs and THORChain
  • Sanity check sending and receiving is working
  • Transaction parsing is working

Engineering

Operations

  • 🏁 My feature is behind a flag and doesn't require operations testing (yet)

Screenshots (if applicable)

THORChain hardfork in progress at the time of opening, will need testing once hardfork is completed and chain is back online.

Lost all screenshots of testing, but tx history shows success for the following:

  • ERC20 approvals
  • EVM same-chain swaps on cowswap + 0x
  • EVM cross-chain swap on lifi
  • rFOX stake
  • rFOX unstake
  • Add ETH/FOX liquidity to uniswap
  • Stake ETH/FOX lP token to fox staking

image

@woodenfurniture woodenfurniture marked this pull request as ready for review September 5, 2024 00:09
@woodenfurniture woodenfurniture requested a review from a team as a code owner September 5, 2024 00:09
@gomesalexandre gomesalexandre self-requested a review September 5, 2024 07:27
gomesalexandre
gomesalexandre reviewed Sep 5, 2024
View reviewed changes
packages/contracts/src/abis/uniswapV2Pair.ts Outdated
@@ -1,4 +1,4 @@
export const IUniswapV2Pair = [
export const UNISWAP_V2_PAIR_ABI = [
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we keep the original naming here so that we don't lose the I prefix? Or at least add it back as Interface somewhere in the name of this ABI 🙏🏽

Copy link
Contributor

Choose a reason for hiding this comment

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

c30ad7c ✅

Copy link
Contributor

Choose a reason for hiding this comment

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

can you explain your reasoning here @gomesalexandre? the abi is for the contract which may or may not implement interfaces, but doesn't seem relevant for abi naming and feels like it should match the contract name realistically anyways.

Copy link
Contributor

Choose a reason for hiding this comment

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

Pretty much that i.e the fact that this is taken from IUniswapV2Pair.sol! https://github.com/Uniswap/v2-core/blob/master/contracts/interfaces/IUniswapV2Pair.sol

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess I am saying the abi is for https://github.com/Uniswap/v2-core/blob/master/contracts/UniswapV2Pair.sol which is the actual contract. UNISWAP_V2_PAIR_ABI is actually more accurate imo. Not a huge deal, but saw it added to the bep20 abi as well and just don't think we should have it in general.

gomesalexandre
gomesalexandre reviewed Sep 5, 2024
View reviewed changes
src/lib/investor/investor-foxy/api/api.ts
@@ -15,7 +15,7 @@ import { ethers } from 'ethers'
import { toLower } from 'lodash'
import { getAddress } from 'viem'
import { bn, bnOrZero } from 'lib/bignumber/bignumber'
import { MAX_ALLOWANCE } from 'lib/investor/constants'
import { MAX_ALLOWANCE } from 'lib/investor/constants/allowance'
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
import { MAX_ALLOWANCE } from 'lib/investor/constants/allowance'
import { maxUint256 } from 'viem'

Copy link
Member Author

Choose a reason for hiding this comment

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

:crazyeyes2:

Copy link
Contributor

Choose a reason for hiding this comment

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

still applicable to use viem here and then delete lib/investor/constants

gomesalexandre
gomesalexandre reviewed Sep 5, 2024
View reviewed changes
Copy link
Contributor

@gomesalexandre gomesalexandre left a comment

Choose a reason for hiding this comment

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

Baller PR ser - a few comments for additional cleanup/revamped naming, but conceptually super happy with this guy! Will runtime paranoia test once review comments are tackled 🙏🏽

gomesalexandre
gomesalexandre reviewed Sep 10, 2024
View reviewed changes
Copy link
Contributor

@gomesalexandre gomesalexandre left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Sanity runtime test looks sane to me minus one possible regression, tested the below.

Note, no Jam since my 10mn jam got rugged with "Processing"

  • Token Arb bridge (both directions) 🚫
  • ETH Arb bridge (both directions)
  • Native EVM token sends
  • Token sends
  • ERC20 approval (CoW)
  • ERC20 swap (CoW)
  • USDT approval reset
  • Native asset swap (Li.fi)
  • ERC20 THOR Swap
  • Native asset THOR swap
  • ETH Arb bridge (both directions)
  • Savers native asset deposit/withdraw
  • Savers token deposit/withdraw
  • RFOX e2e

As part of the sanity test here, noticed one smolish issue WRT sends styling for long token symbols we will want to tackle in a separate PR

Screenshot 2024-09-10 at 10 55 59

gomesalexandre
gomesalexandre approved these changes Sep 10, 2024
View reviewed changes
Copy link
Contributor

@gomesalexandre gomesalexandre left a comment

Choose a reason for hiding this comment

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

Retested focusing on Arbitrum Bridge, confirmed the previous behaviour re: unable to get Arb Bridge quotes for tokens was an intermittent issue.

Unrelated to this diff, found out that we poll for SAFE Txs when we know the current address isn't a sc address, but this will be fixed in #7525

Screenshot 2024-09-10 at 14 19 53
  • ETH Arb Bridge (both directions) ✅

https://jam.dev/c/8354955a-51f2-4797-82ac-d0b4b28dbd47

  • Token Arb Bridge (both directions) ✅

https://jam.dev/c/dfba760d-1d1d-4d7c-8105-130fe640442d

  • Claims List
image
  • Claim

https://jam.dev/c/2057cada-3007-4a3f-b6f8-db66186736df

kaladinlight
kaladinlight reviewed Sep 11, 2024
View reviewed changes
packages/swapper/src/swappers/ThorchainSwapper/evm/utils/getThorTxData.ts Outdated
Comment on lines 37 to 40
const router = getAddress(inboundAddress.router as string)
const vault = getAddress(inboundAddress.address as string)

if (!router) {
Copy link
Contributor

Choose a reason for hiding this comment

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

non blocking suggestion - this changes error behavior slightly. We can move below our if (!router) check (probably change to if (!inboundAddress.router), then have our custom error message and no more need to cast

Copy link
Member Author

Choose a reason for hiding this comment

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

I prefer not to - this was added by gomes's review feedback and strictly isnt necessary but I'd prefer not to add more change because its really not related to the PR

kaladinlight
kaladinlight reviewed Sep 11, 2024
View reviewed changes
packages/contracts/src/constants.ts Outdated
export const UNISWAP_V2_ROUTER_02_CONTRACT_ADDRESS =
'0x7a250d5630B4cF539739dF2C5dAcb4c659F2488D' as const
// Uniswap V2
export const UNISWAP_V2_ROUTER_02_MAINNET = '0x7a250d5630B4cF539739dF2C5dAcb4c659F2488D' as const
Copy link
Contributor

Choose a reason for hiding this comment

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

contract naming consistency suggestion - at least include CONTRACT or CONTRACT_ADDRESS (contract is fine imo, but see both in here), on all contract address constants.

also a question, is MAINNET intended to suggest the address is the same across all evm networks? or is this talking about ethereum mainnet? non obvious and a bit confusing imo if we want to clean up that naming convention.

Copy link
Member Author

Choose a reason for hiding this comment

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

also a question, is MAINNET intended to suggest the address is the same across all evm networks? or is this talking about ethereum mainnet? non obvious and a bit confusing imo if we want to clean up that naming convention.

Unsure as this is unrelated to this PR and is from existing implementation. I'm tempted to leave it as-is unless necessary to change in case it makes things confusing in wherever these are used

Copy link
Member Author

Choose a reason for hiding this comment

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

I've updated the address naming to be consistent throughout though :)

Copy link
Contributor

Choose a reason for hiding this comment

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

also a question, is MAINNET intended to suggest the address is the same across all evm networks? or is this talking about ethereum mainnet? non obvious and a bit confusing imo if we want to clean up that naming convention.

Unsure as this is unrelated to this PR and is from existing implementation. I'm tempted to leave it as-is unless necessary to change in case it makes things confusing in wherever these are used

For posterity, while the name is unchanged from the existing implementation, the difference was the constants were defined inline within unchained-client/src/evm/ethereum in at least some cases in which MAINNET made sense in this context as being directly related to eth mainnet (also considering there was ropsten constants). In the global contracts package, this context is lost and mainnet become vague considering all of our chains are mainnet.

0xApotheosis
0xApotheosis reviewed Sep 13, 2024
View reviewed changes
Copy link
Contributor

@0xApotheosis 0xApotheosis left a comment

Choose a reason for hiding this comment

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

Diff looks good expect for the loss of typing for the ABIs.

packages/contracts/src/abis/rfox.ts Show resolved Hide resolved
packages/swapper/src/swappers/ThorchainSwapper/evm/utils/getThorTxData.ts Outdated Show resolved Hide resolved
packages/swapper/src/swappers/ThorchainSwapper/evm/utils/getThorTxData.ts Outdated Show resolved Hide resolved
0xApotheosis
0xApotheosis approved these changes Sep 13, 2024
View reviewed changes
Copy link
Contributor

@0xApotheosis 0xApotheosis left a comment

Choose a reason for hiding this comment

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

Solid PR man. Code looking solid, leaning on the runtime tests from gomes and soon to be from ops to get this in.

@0xApotheosis 0xApotheosis merged commit 76df0f3 into develop Sep 13, 2024
3 checks passed
@0xApotheosis 0xApotheosis deleted the abi-addresses-cleanup branch September 13, 2024 02:05
@woodenfurniture woodenfurniture mentioned this pull request Sep 16, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kaladinlight kaladinlight kaladinlight left review comments

@gomesalexandre gomesalexandre gomesalexandre approved these changes

@0xApotheosis 0xApotheosis 0xApotheosis approved these changes

Assignees
No one assigned
Labels
high risk
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Post contracts package cleanup
4 participants
@woodenfurniture @gomesalexandre @kaladinlight @0xApotheosis