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

DX Improvement: add local abi source option #65

Open
agcty opened this issue Dec 28, 2021 · 7 comments
Open

DX Improvement: add local abi source option #65

agcty opened this issue Dec 28, 2021 · 7 comments

Comments

@agcty
Copy link

agcty commented Dec 28, 2021

Fetching ABIs from etherscan or sourcify is great but only works if the contracts are already deployed and verified. But how do you work with contracts that are not yet deployed?

Would like to build different frontend environments: local (npx hardhat node) / staging (e.g goerli) / production (e.g mainnet)

I'm working on some local smart contracts with hardhat and don't want to deploy them just yet, I just want to play around locally and have my frontend pick up all the right types. Seems like I can't do that at the moment (or haven't found a way by looking through the docs).

Something like that would improve DX by 100x

const config: EthSdkConfig = {
  contracts: {
    mainnet: {
      dai: "0x4a436073552044D5f2f49B176853ad3Ad473d9d6",
    },
    goerli: {
      dai: "0x3a436073552044D5f2f49B176853ad3Ad473d9d6",
    },
    local: {
      dai: {address: "0x5a436073552044D5f2f49B176853ad3Ad473d9d6", abi: "../dai.json"},
    },
  },
  networkIds: {
    local: 1337
  },
};

Basically allowing you to opt in to use a local abi for any contract. There is also the possibility of defining that all local contracts should fetch the abi from a local source and do some filename lookup but having a granular approach that lets you choose where the abi is coming from for each contract is better IMO because you might be working with some contract that is not verified for some reason but you still have the abi. So you could mix verified contracts on mainnet with unverified ones where you still have the abi for example. (Note: All contracts should be verified but this use case is still valid IMO)

For context, to use this in a frontend I'd probably create a custom hook that gets the current network name from an env file and returns the correct SDK.

Like this (pseudo-code):

function useSDK() {
  const { library } = useWeb3React();

  // if a signer is injected by web3-react via Metamask use that, if not, use a default provider
  const signerOrProvider = library ? library : getDefaultProvider();

  if (process.env.NEXT_PUBLIC_NETWORK === "mainnet") {
    return getMainnetSDK(signerOrProvider);
  }

  if (process.env.NEXT_PUBLIC_NETWORK === "goerli") {
    return getGoerliSDK(signerOrProvider);
  }

  if (process.env.NEXT_PUBLIC_NETWORK === "local") {
    return getLocalSDK(signerOrProvider);
  }
}

Let me know if I can help in any way to support this :)

@agcty
Copy link
Author

agcty commented Dec 28, 2021

Just found a potential workaround in another issue:

Thanks for the report -- we'll investigate this.

As a quick workaround, you can simply copy USDC abi and place it manually in abi dir (eth-sdk/abi/{network}/{name}.json)

Originally posted by @krzkaczor in #49 (comment)

Guess that works but it's just a workaround, would be nice to have an officially supported way of using local abi sources.

@krzkaczor
Copy link
Member

@agcty I feel your pain. This is definitely something that we should support but the problem is that on localhost, often, all addresses are random. I thought that in this case, we should expose more general methods like getSDKWithoutAddresses that would take a provider and am object with all addresses. Then basically you would configure SDK in code rather than in config. WDYT?

@agcty
Copy link
Author

agcty commented Jan 2, 2022

Understand what you mean now, the problem is rooted a little deeper than I thought.

I think there are 2 approaches eth-sdk could potentially take.

Approach 1 (current approach):
The way it works now is that the SDK returns an instantiated representation of the contracts already, tightly coupled to the addresses in the config object which has some benefits but also some downsides.

Benefits

  • don't need to instantiate class again, can just use it with get{NetworkName}SDK()
  • all the addresses in one file
  • generally makes sense to tightly couple addresses to contracts
  • very easy to use

Downsides

  • IMO not very flexible
  • currently can't use a contract twice except having two different addresses pointing to the same contract
  • can only really define contracts at build time
  • cannot create contracts with the same type at runtime

I think the best way would be to allow for maximum flexibility while still maintaining ease of use by offering a function that returns only the classes but not already the instantiation.

Approach 2:
I propose a new generalized function getSDK():

  1. you still have a config file with addresses or local abi paths (only use case is pointing to abis, addresses have no other meaning)
  2. running eth-sdk fetches the ABIs either from an address or locally
  3. Instead of returning the instantiated classes, it just returns the classes

This would allow for maximum flexibility because you'd be able to create an instantiation of a contract at runtime like this:

const { MyContract } = getSDK()
new MyContract("0x32323....", provider)

In addition, this would allow you to use different providers if needed. Pretty nice free added benefit imo.

Benefits

  • maximum flexibility
  • can easily create contracts of the same type but with different addresses*
  • can use different providers easily
  • I think this is an especially important point and will only gain in importance as web3 grows bigger. Imagine you want to build a decentralized frontend for searching and filtering through different forks of uniswap, this would be really easy to build with the generalized getSDK but not at all possible with how it currently works.

Now we made it much more flexible but was this at the cost of ease of use? No.

eth-sdk could just offer a wrapper function getInstantiatedSDK() (literally getSDKWithoutAddresses but just renamed) that takes a list of addresses as a parameter and uses getSDK under the hood (would also make it backwards compatible).

Let me know what you think, also always happy to jump on a call if something doesn't make sense!

Edit: Would also solve #43

@krzkaczor
Copy link
Member

krzkaczor commented Jan 9, 2022

Thanks for spending time writing this down. I like this proposal. It's inline with how I was thinking about it. The problem is that each network can list different addresses + some contracts might now be even deployed on any network and requires providing ABI manually (we could allow specifying abi path instead of address for a special network called "local").

It would be awesome if you would want to develop this feature. As a next step I would love to see a detailed written proposal and discuss with exact code examples etc.

@agcty
Copy link
Author

agcty commented Jan 12, 2022

@krzkaczor yes would love to spend some time on this and write a proposal, give me some time :)

@beamercola
Copy link

Crazy idea people, but is it easier to mock the etherscan api on the localhost/hardhat side? Like a Hardhat plug-in that spins up express server on a port

Keeps this beautiful library focused and isolates the use case for other potential applications

@gas1cent
Copy link

gas1cent commented Jul 6, 2022

Adding the ABIs manually worked, but the directory name must be plural, eth-sdk/abis

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

No branches or pull requests

4 participants