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: add support for unverified uniswap v3 pools [APE-1616] #109

Closed
wants to merge 1 commit into from

Conversation

salparadi
Copy link

What I did

Allowed unverified Uniswap v3 contracts that are designated as "similar code" to pull a generic Uniswap v3 ABI instead of failing.

How I did it

Created a new Client for the proxy endpoint. Specifically to pass in a tx_hash. I recognize there are other data pieces you would want to pass in to proxy actions, but for this use case this seemed simplest.

Created a new GetTransactionByHashResponse dataclass

Created a new get_transaction_by_hash function which returns GetTransactionByHashResponse

Adjusted the ape_etherscan/explorer/get_contract_type function to run a check if the abi is not properly returned by the getsourcecode endpoint (the response is not JSON).

If it is not valid JSON, meaning it is a string saying Contract source code not verified, get the contract creation data, which gives us the transaction hash of the contract creation.

call the proxy/eth_getTransactionByHash endpoint using the new Proxy Client and retrieve the transaction information

Check the to field from the transaction against the known Uniswap V3: Positions NFT address, which is the to address of all Uniswap v3 pool initializations.

Load the generic Uniswap v3 ABI constant from ape_etherscan/utils

Continue the get_contract_type logic.

How to verify it

Load an ape console and try to get an unverified Uniswap v3 contract. Example:

contract = Contract('0x86E69D1AE728c9Cd229F07bBf34E01bF27258354')

It should have no errors and the contract should be available.

Checklist

  • Passes all linting checks (pre-commit and CI jobs)
  • New test cases have been added and are passing
  • Documentation has been updated
  • PR title follows Conventional Commit standard (will be automatically included in the changelog)

@vany365 vany365 changed the title feat: add support for unverified uniswap v3 pools feat: add support for unverified uniswap v3 pools [APE-1616] Dec 13, 2023
Copy link
Member

@antazoey antazoey left a comment

Choose a reason for hiding this comment

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

Love it. But I need to circle back but proper review.
Will this work for all proxy types? Ape has a proxy decoding system we could leverage.
Sorry for the quick review - getting Ape 0.7 out!

thank you kindly for this contribution.

if not (abi_string := source_code.abi):
return None

try:
abi = json.loads(abi_string)
except JSONDecodeError as err:
logger.error(f"Error with contract ABI: {err}")
return None
contract_creation = contract_client.get_creation_data()
Copy link
Member

Choose a reason for hiding this comment

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

thought: much of this proxy checking here could be abstracted into a helper method on this instance to avoid so much stuff happening in the except block

if not hasattr(contract_creation[0], "txHash"):
return None

tx_hash = contract_creation[0].txHash
Copy link
Member

Choose a reason for hiding this comment

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

idea: could use walrus like you do during if not (to_address := transaction_by_hash.toAddress): below

if not self.conversion_manager.is_type(to_address, AddressType):
to_address = self.conversion_manager.convert(str(to_address), AddressType)

if to_address == UNISWAP_V3_POSITIONS_NFT:
Copy link
Member

Choose a reason for hiding this comment

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

would like @fubuloubu 's thought here, else I will return after I have reread the description of the PR and done research...

gut feeling at first review: it this something that ape's proxy system should be handling?

Copy link
Member

Choose a reason for hiding this comment

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

I suppose would like it that more protocol-specific handling end up in the protocol SDK libraries, so we don't end up cluttering the plugins/core a lot

But this is popular, so could be convinced

Copy link

This pull request is considered stale because it has been open 30 days with no activity. Remove stale label, add a comment, or make a new commit, otherwise this PR will be closed in 5 days.

@github-actions github-actions bot added the stale label Jan 19, 2024
return None

proxy_client = self._client_factory.get_proxy_client(tx_hash)
transaction_by_hash = proxy_client.get_transaction_by_hash()
Copy link
Member

Choose a reason for hiding this comment

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

Instead of making a new client, you could do transaction_by_hash = self.provider.ge_receipt(tx_hash)

@github-actions github-actions bot removed the stale label Jan 20, 2024
Copy link

This pull request is considered stale because it has been open 30 days with no activity. Remove stale label, add a comment, or make a new commit, otherwise this PR will be closed in 5 days.

@github-actions github-actions bot added the stale label Feb 20, 2024
Copy link

This PR was closed because it has been inactive for 35 days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants