Integrations/uni v3 #49
Conversation
There was a problem hiding this comment.
Great work, but there are some things that we could look into.
Taking the test as an example of the usage, it feels more like a standalone ape script than something that should be integrated with Agents, the agents handles the default sender, the chain and the instantiation of the contracts which is done manually in this script and in the uniswap classes.
Maybe some things that we could do:
- Create a base class for the uniswap contracts so we can have common functionality in the instantiation, logging, utils...
- Think about de DX like for example calling directly to the function rather than calling the underlying contract (
uni.foo(...)vsuni.router.foo(...) - If we are thinking of having multiple integrations, we might need to define how to develop the new ones to focus on extensibility and modularity
|
|
||
| from giza_actions.integrations.uniswap.uniswap import Uniswap | ||
|
|
||
| networks.parse_network_choice(f"ethereum:mainnet-fork:foundry").__enter__() |
There was a problem hiding this comment.
I would rather use the with statement rather than the __enter__ method:
with networks.parse_network_choice(f"ethereum:mainnet-fork:foundry"):
...There was a problem hiding this comment.
Keep the folder name in lower: giza_actions/integrations/uniswap/assets/erc20.json
There was a problem hiding this comment.
I'm guessing that these .json are basically the contracts definitions, right?
There was a problem hiding this comment.
We could consider the addresses as constants and we could add it here rather than having two separate files
There was a problem hiding this comment.
I didn't want to clutter the constants.py with a long dictionary and whenever I'm working with a web3 project, I find it helpful to have one place with all the addresses so I can go to the explorer and get a better feel of a contract... having said that, happy to move to constants.py if you deem it better
There was a problem hiding this comment.
Missing docstrings and type hints
| uni = Uniswap(sender=sender, version=3) | ||
| pool = uni.get_pool(token0, token1, fee) | ||
| price = pool.get_pool_price(invert=True) | ||
| print(f"--------- Pool Price: {price}") |
There was a problem hiding this comment.
Even if its for testing purposes i would rather use logging than print:
import logging
logger = logging.getLogger(__name__)
logger.info(f"--------- Pool Price: {price}")| @@ -0,0 +1,39 @@ | |||
| ADDRESSES = { | |||
| # mainnet | |||
| 1: { | |||
There was a problem hiding this comment.
Why not using names directly, rather than the chain_id?
There was a problem hiding this comment.
To avoid errors when testing with local forks. If I used the name (e.g. ethereum:mainnet) but I wanted to test on e.g. ethereum:mainnet-fork:foundry, I would need to add another parsing method. This way I have a unique id that will always work
| ADDRESSES = { | ||
| # mainnet | ||
| 1: { | ||
| 3: { |
There was a problem hiding this comment.
Why there is always this 3 as a key in all the addresses?
There was a problem hiding this comment.
uniswap version 3. I don't have v2 support yet but it definitely should be one of the next todos
| address, abi=os.path.join(os.path.dirname(__file__), "ASSETS/pool.json") | ||
| ) | ||
| self.sender = sender | ||
| self.token0 = Contract( |
There was a problem hiding this comment.
Are all of these variables supposed to be used by the user? We need to think what we tell the user "This is for you to use" or "This is 'private' and use at your own risk"
| ### Quoter ### | ||
| amount_in = int(1e8) | ||
| # calling using pool object | ||
| amount_out = uni.quoter.quote_exact_input_single(amount_in, pool=pool) |
There was a problem hiding this comment.
I feel like using uni.quoter or uni.nft_manager everytime that we want to execute makes the Uniswap class a little bit useless as we could just do the init by ourselves of the other classes and call directly de quoter or router.
Maybe we need to think about how we organize the code
… frontend for all contract interactions, added type hints
This PR implements an ape-based sdk for Uniswap V3