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

Enhancement/ethers support #147

Closed
wants to merge 11 commits into from
Closed

Conversation

lnbc1QWFyb24
Copy link
Collaborator

This PR adds support for projects using ethers.js. Devs can now pass in ethers.js in the config instead of web3.js and also decorate ethers.js instantiated contracts with our Contract function.

This needs some thorough testing as there are some pretty major changes that can affect existing projects that are using web3.ethers contracts require that the whole contract object (not just the method be passed down in to our preflight checks, which required some architectural changes.

I am intermittently getting an issue with MetaMask where the txPromise doesn't resolve until the transaction has been mined as opposed to when a transaction hash is available upon user approval, which causes problems with out notification system. This is issue is referenced here: ethers-io/ethers.js#372
I am not sure that there is anything that we can do about this. But would like to see if @cmeisl and @liamaharon experience the same issue.

Also something to consider would be to find an open source dapp frontend that is currently using ethers.js and invest some time integrating this version of assist to see if there is anything that this PR is missing.

@lnbc1QWFyb24 lnbc1QWFyb24 added the enhancement New feature or request label May 7, 2019
@liamaharon
Copy link
Contributor

This is issue is referenced here: ethers-io/ethers.js#372
I am not sure that there is anything that we can do about this. But would like to see if @cmeisl and @liamaharon experience the same issue.

  1. I can confirm I have the same issue when using the signer injected by MM.

  2. In your blocknative-demo branch if I decorate the contract with an internal signer instead of the MM signer like this:

  const provider = ethers.getDefaultProvider('rinkeby')
  const wallet = new ethers.Wallet(<priv-key>, provider)

  let highFiveContract = new ethers.Contract(
    caddr,
    abi,
    wallet
  )

calling decoratedContract.highFive results with 'Transaction waiting for you to confirm' popping up for ~0.25s then being replaced by 'Sending high five'. Maybe we could detect when the signer isn't MM, and in that case not show the 'Tx waiting for you to confirm' popup?

  1. Update readme?

@@ -253,8 +330,25 @@ export function getAccountBalance() {
})
}

export function getContractMethod({ contractObj, methodName, overloadKey }) {
return state.legacyWeb3
Copy link
Contributor

@liamaharon liamaharon May 9, 2019

Choose a reason for hiding this comment

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

It can take me a while to internalise nested ternary, I reckon it might be quicker for new eyes to understand what's happening here if it was written with conditionals instead?

@lnbc1QWFyb24
Copy link
Collaborator Author

Closing this for the moment, can revisit once ethers v5 is out

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants