Skip to content

Postgres wallet table#634

Merged
slundqui merged 14 commits intodelvtech:mainfrom
slundqui:postgres_agents
Jul 6, 2023
Merged

Postgres wallet table#634
slundqui merged 14 commits intodelvtech:mainfrom
slundqui:postgres_agents

Conversation

@slundqui
Copy link
Copy Markdown

@slundqui slundqui commented Jul 6, 2023

No description provided.

@vercel
Copy link
Copy Markdown

vercel Bot commented Jul 6, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
elf-simulations ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 6, 2023 8:04pm

Copy link
Copy Markdown
Contributor

@dpaiton dpaiton left a comment

Choose a reason for hiding this comment

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

I don't think the pattern for get_funding_contract is what we want. My understanding is that the addresses.base_token address and the ERC20Mintable ABI arguments are tied to a "funding account". These are tied together inside Hyperdrive, which takes a "base token" that must follow the ERC20 standard. I might have this wrong, but if I'm right then I think we want to hardcode these things together on our end as well. @sentilesdal let us know if this sounds correct to you.

my proposal: I think the process of setting the constant FUNDING_ABI_FILE_PATH and passing that as an argument to get_funding_contract is an unnecessary abstraction given above. Assuming I'm right, an alternative route is to load all of the ABIs at the outset and then look for the correct ABI for a given token address. This might also remove the need to specify any of the *_ABI_FILE_PATH variables, replacing them with a single ABI_FOLDER argument. So the new workflow is to run contract_interface.load_all_abis(ABI_FOLDER) and then tie together the base address with the ERC20Mintable contract:

# Do these outside of the function since they may take time & should only be done once
addresses = fetch_addresses_from_url(contracts_url)
abis = load_all_abis(abi_folder)

def get_funding_contract(web3: Web3, abis: dict, addresses: HyperdriveAddressesJson) -> Contract:
    """Get the base contract for a given abi"""
    if "ERC20Mintable" not in abis:
        raise AssetionError("ERC20 ABI for minting base tokens was not provided")
    # choose the ABI from the list
    state_abi =abis["ERC20Mintable"]
    # get contract instance of hyperdrive
    # NOTE: not a "hyperdrive_contract" because we use that var name elsewhere to refer to a Contract from the "IHyperdrive" ABI
    funding_contract: Contract = web3.eth.contract(
        address=address.to_checksum_address(addresses.base_token), abi=state_abi
    ) 
    return funding_contract

Comment thread elfpy/data/contract_interface.py
Comment thread elfpy/data/contract_interface.py Outdated
Comment thread elfpy/data/contract_interface.py Outdated
Comment thread elfpy/data/db_schema.py
Comment thread elfpy/data/postgres.py
Comment thread elfpy/data/postgres.py Outdated
@sentilesdal
Copy link
Copy Markdown
Contributor

I don't think the pattern for get_funding_contract is what we want. My understanding is that the addresses.base_token address and the ERC20Mintable ABI arguments are tied to a "funding account". These are tied together inside Hyperdrive, which takes a "base token" that must follow the ERC20 standard. I might have this wrong, but if I'm right then I think we want to hardcode these things together on our end as well. @sentilesdal let us know if this sounds correct to you.

my proposal: I think the process of setting the constant FUNDING_ABI_FILE_PATH and passing that as an argument to get_funding_contract is an unnecessary abstraction given above. Assuming I'm right, an alternative route is to load all of the ABIs at the outset and then look for the correct ABI for a given token address. This might also remove the need to specify any of the *_ABI_FILE_PATH variables, replacing them with a single ABI_FOLDER argument. So the new workflow is to run contract_interface.load_all_abis(ABI_FOLDER) and then tie together the base address with the ERC20Mintable contract:

# Do these outside of the function since they may take time & should only be done once
addresses = fetch_addresses_from_url(contracts_url)
abis = load_all_abis(abi_folder)

def get_funding_contract(web3: Web3, abis: dict, addresses: HyperdriveAddressesJson) -> Contract:
    """Get the base contract for a given abi"""
    if "ERC20Mintable" not in abis:
        raise AssetionError("ERC20 ABI for minting base tokens was not provided")
    # choose the ABI from the list
    state_abi =abis["ERC20Mintable"]
    # get contract instance of hyperdrive
    # NOTE: not a "hyperdrive_contract" because we use that var name elsewhere to refer to a Contract from the "IHyperdrive" ABI
    funding_contract: Contract = web3.eth.contract(
        address=address.to_checksum_address(addresses.base_token), abi=state_abi
    ) 
    return funding_contract

Yeah this is fine. I'm not too particular here since there's no useful way to make this code more generic. loading addresses and abis outside the defs makes sense. It would be nice to have functions that are like:

def get_token_contract(web3: Web3, abi: ABI, address: str) -> Contract:

but it might not end up actually doing anything useful.

I do think we should start breaking up contract_interface.py into generic contract things only, and then hyperdrive things should live elsewhere, like elfpy/contracts/hyperdrive.py

Copy link
Copy Markdown
Contributor

@sentilesdal sentilesdal left a comment

Choose a reason for hiding this comment

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

looks great!

@slundqui slundqui merged commit 4db89ef into delvtech:main Jul 6, 2023
@slundqui slundqui deleted the postgres_agents branch July 6, 2023 20:08
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.

4 participants