UMAprotocol / protocol

UMA Protocol Running on Ethereum
https://uma.xyz
GNU Affero General Public License v3.0
369 stars 182 forks source link

Make contract artifacts available from `core` to all other Lerna packages #1732

Closed nicholaspai closed 4 years ago

adrianmcli commented 4 years ago

Here's one way to do it:

  1. Put all smart contracts into one package called @umaprotocol/smart-contracts.
  2. Inside that package's truffle-config.js, set contracts_build_directory to artifacts.
  3. In any other package within the Lerna repo, you can now do this:
import MyContract from "@umaprotocol/smart-contracts/artifacts/MyContract.json"
mrice32 commented 4 years ago

But then you have to manually initialize the truffle-contract abstraction, right? If we could find a way to make these contracts first class citizens when truffle is initialized in the other packages, that would be ideal. Not sure if there's a clean way to do that outside of weird symlinks, though. Thoughts?

mrice32 commented 4 years ago

I'd even be okay if we had to publish some sort of EthPM package or something to do this. We could always export the raw artifacts in addition to that to make it cross-compatible with other import setups.

adrianmcli commented 4 years ago

But then you have to manually initialize the truffle-contract abstraction, right?

I think it's a good idea to move away from the truffle-contract abstraction, it means we're less coupled to the Truffle Way™️ and also allows us to take full advantage of the more widely-used Web3.js contract abstraction and perhaps later on Ethers.js.

mrice32 commented 4 years ago

This might just work with truffle: https://www.trufflesuite.com/docs/truffle/getting-started/package-management-via-npm. Seems that you can just export the files from core, install the @umaprotocol/core in the second package under node_modules and truffle will pick it up when you reference it.

My only worry is that yarn will dump it under node_modules a directory up (at root) and truffle won't be able to find it.

mrice32 commented 4 years ago

I actually needed this to support lerna-izing financial-templates-lib, and I can verify that the contract artifacts are accessible by doing require("@umaprotocol/core/build/contracts/SomeContract.json"). It's pretty nice, too because I think this will also hook into artifacts.require("@umaprotocol/core/SomeContract"), but I haven't used that yet.

nicholaspai commented 4 years ago

I can verify that the contract artifacts are accessible by doing require("@umaprotocol/core/build/contracts/SomeContract.json").

This syntax really improves readability. Getting less coupled from Truffle is a good goal as well. I feel that we use it in many cases to access (1) the web3 property and (2) the artifacts.require() abstraction.

adrianmcli commented 4 years ago
require("@umaprotocol/core/build/contracts/SomeContract.json")

With contracts_build_directory set to dist, this can be reduced to:

require("@umaprotocol/core/dist/SomeContract.json")

A little history on why Truffle's default contract artifact directory is build/contracts:

Originally, Truffle would compile everything, including a frontend for you that includes your HTML and JS files that would be copied into the build/* directory. That's why the contract artifacts are in the contracts folder one level deeper: to separate them from your frontend. But this is obviously no longer necessary since Truffle doesn't do that for you anymore. However, we're still left with these vestigial pieces.

mrice32 commented 4 years ago

@nicholaspai Apparently artifacts.require() is broken for either scoped packages (or maybe all node_modules packages), but a PR was sent out today to fix it.

Not super opinionated about how we import the contracts. Doing it without artifacts.require() is pretty clunky, so if we decided to move away from it, we should probably write our own wrapper that pre-constructs the web3 interface with the abi and deployed address from networks/*.json or the build artifacts (if those are universal in structure).

nicholaspai commented 4 years ago

@nicholaspai Apparently artifacts.require() is broken for either scoped packages (or maybe all node_modules packages), but a PR was sent out today to fix it.

Not super opinionated about how we import the contracts. Doing it without artifacts.require() is pretty clunky, so if we decided to move away from it, we should probably write our own wrapper that pre-constructs the web3 interface with the abi and deployed address from networks/*.json or the build artifacts (if those are universal in structure).

What's nice about importing contracts via artifacts.require() versus truffle-contract is that the contract methods are easier to access: contract.someMethod() versus contract.methods.someMethod().send(). The contract address is also available contract.address versus contract.options.address. Besides this I haven't noticed any difference.

adrianmcli commented 4 years ago

FYI: Ethers.js also has the more ergonomic API:

If you want to instantiate your own, this is what you'd do (I've done this several times in the past for different projects):

// Web3.js
const contractFromArtifact = async (artifact: Artifact, web3: Web3) => {
  const networkId = await web3.eth.net.getId();
  const deployedAddress =
    artifact.networks &&
    artifact.networks[networkId] &&
    artifact.networks[networkId].address;

  const instance = new web3.eth.Contract(artifact.abi, deployedAddress);
  return instance;
};
// Ethers.js
const contractFromArtifact = async (artifact: Artifact, wallet: Wallet) => {
  const networkId = await wallet.provider.getNetwork();
  const deployedAddress =
    artifact.networks &&
    artifact.networks[networkId] &&
    artifact.networks[networkId].address;

  const instance = new ethers.Contract(deployedAddress, artifact.abi, wallet);
  return instance;
};

You can also add custom logic to get your deployed addresses from somewhere else of course.

mrice32 commented 4 years ago

@nicholaspai Apparently artifacts.require() is broken for either scoped packages (or maybe all node_modules packages), but a PR was sent out today to fix it. Not super opinionated about how we import the contracts. Doing it without artifacts.require() is pretty clunky, so if we decided to move away from it, we should probably write our own wrapper that pre-constructs the web3 interface with the abi and deployed address from networks/*.json or the build artifacts (if those are universal in structure).

What's nice about importing contracts via artifacts.require() versus truffle-contract is that the contract methods are easier to access: contract.someMethod() versus contract.methods.someMethod().send(). The contract address is also available contract.address versus contract.options.address. Besides this I haven't noticed any difference.

What I mean is that you have to pull in the abi and address from the json file and then call new to create the web3 contract instance. It’s just more boilerplate to get set up. Not terrible by any means tho. Agree that the accessor api is basically the same — in some respects I actually like the web3 one more because you can construct the transaction w args and then send it in a separate step.

nicholaspai commented 4 years ago

@nicholaspai Apparently artifacts.require() is broken for either scoped packages (or maybe all node_modules packages), but a PR was sent out today to fix it. Not super opinionated about how we import the contracts. Doing it without artifacts.require() is pretty clunky, so if we decided to move away from it, we should probably write our own wrapper that pre-constructs the web3 interface with the abi and deployed address from networks/*.json or the build artifacts (if those are universal in structure).

What's nice about importing contracts via artifacts.require() versus truffle-contract is that the contract methods are easier to access: contract.someMethod() versus contract.methods.someMethod().send(). The contract address is also available contract.address versus contract.options.address. Besides this I haven't noticed any difference.

What I mean is that you have to pull in the abi and address from the json file and then call new to create the web3 contract instance. It’s just more boilerplate to get set up. Not terrible by any means tho. Agree that the accessor api is basically the same — in some respects I actually like the web3 one more because you can construct the transaction w args and then send it in a separate step.

Another problem with instantiating contracts in a client via web3/ethers, is that some of our common/ scripts expect contract methods to be called contract.method() and have addresses accessible via contract.address.

mrice32 commented 4 years ago

Another problem with instantiating contracts in a client via web3/ethers, is that some of our common/ scripts expect contract methods to be called contract.method() and have addresses accessible via contract.address.

Yeah, that's a good point.

kendricktan commented 4 years ago

I actually like the web3 one more because you can construct the transaction w args and then send it in a separate step.

You can do a partial function to mimic that behavior if you'd like.

What I mean is that you have to pull in the abi and address from the json file and then call new to create the web3 contract instance. It’s just more boilerplate to get set up. Not terrible by any means tho. Agree that the accessor api is basically the same — in some respects I actually like the web3 one more because you can construct the transaction w args and then send it in a separate step.****

To overcome that we can make the core directory an npm package itself, and create some helper functions that automatically exports (either a web3 or ethers contract instance). Sort of how like Synthetix does it, but we will return a contract instance instead of the JSON data blob to remove the repetition. E.g.

const uma = require('@uma/smart-contracts');

// Returns a contract instance
uma.getContract({ network: 'mainnet', contract: 'Voting' })

Another problem with instantiating contracts in a client via web3/ethers, is that some of our common/ scripts expect contract methods to be called contract.method() and have addresses accessible via contract.address.

I think we should be able to do a regex replace to avoid the mechanical turk :sweat_smile:

FYI: Ethers.js also has the more ergonomic API:

I agree, if we are not already using web3js, and is starting from a clean slate, I personally would prefer ethers as it has a more ergonomic API (not to mention web3 depends on ethers....)

mrice32 commented 4 years ago

This is done.