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

Adds ViemSigner compatibility layer #294

Closed
wants to merge 39 commits into from
Closed

Adds ViemSigner compatibility layer #294

wants to merge 39 commits into from

Conversation

douglance
Copy link
Contributor

@douglance douglance commented Jun 12, 2023

  • AssetBridgers can be instantiated with a provider from ethers v5, v6, web3, or viem.
    • Question: Do we want to remove the support for ethers v6 and web3?
  • Signing can be done with ethers v5 or viem signers.
    • consuming packages must instantiate a ViemSigner using createViemSigner before passing in the signer to the sdk.

@cla-bot cla-bot bot added the cla-signed label Jun 12, 2023
@douglance douglance changed the title Patches EthBridger.from to expand provider support Updates EthBridger.from to expand provider support Jun 14, 2023
@douglance douglance changed the title Updates EthBridger.from to expand provider support Adds ViemSigner compatibility layer Dec 4, 2023
Copy link

cla-bot bot commented Dec 11, 2023

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Default Name.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email [email protected]
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

@cla-bot cla-bot bot removed the cla-signed label Dec 11, 2023
@fionnachan fionnachan self-requested a review December 11, 2023 19:06
.env-sample Show resolved Hide resolved
.vscode/launch.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
scripts/testSetup.ts Outdated Show resolved Hide resolved
scripts/testSetup.ts Outdated Show resolved Hide resolved
scripts/testSetup.ts Outdated Show resolved Hide resolved
Comment on lines +39 to +56
export const getWeb3Url = (provider: Providerish) => {
if (isHttpProvider(provider)) {
// @ts-expect-error - private member
if (provider.clientUrl) {
// @ts-expect-error - private member
return provider.clientUrl
// @ts-expect-error - private member
} else if (provider.currentProvider && provider.currentProvider.clientUrl) {
// @ts-expect-error - private member
return provider.currentProvider.clientUrl
// @ts-expect-error - private member
} else if (provider._socketPath) {
// @ts-expect-error - private member
return provider._socketPath
}
}
return undefined
}
Copy link
Member

Choose a reason for hiding this comment

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

what's happening here with the @ts-expect-error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We access private fields, but they're not true private fields. We can still access them, but TS doesn't like us accessing them.

src/lib/utils/universal/providerTransforms.ts Show resolved Hide resolved
tests/integration/universal/universalSigner.test.ts Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

i think we should also have unit tests for this one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added unit tests 77290dd

@douglance douglance marked this pull request as ready for review December 28, 2023 19:37
Comment on lines +255 to +266
chain: {
...mainnet,
id: 1337,
rpcUrls: {
default: {
http: [config.ethUrl],
},
public: {
http: [config.ethUrl],
},
},
},
Copy link
Member

Choose a reason for hiding this comment

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

why wouldn't you use ethLocal here?

const hash = await this.publicClient.sendRawTransaction({
serializedTransaction,
const hash = await this.walletClient.sendTransaction({
...request,
Copy link
Member

Choose a reason for hiding this comment

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

Is this to create a copy and make sure request is not changed?
It is viem's responsibility to ensure the function doesn't create side effects to request, and by the look of its code we should pass request directly
https://github.com/wevm/viem/blob/main/src/actions/wallet/sendTransaction.ts#L121

unless we are passing additional params than request, I don't see why we should spread it

Comment on lines +16 to +45
describe('Provider Utilities', () => {
describe('ethers v5', () => {
it('should return the URL for an actual EthersV5 provider', () => {
const provider = new JsonRpcProvider('http://localhost:8545')
const url = getEthersV5Url(provider)
expect(url).to.be.equal('http://localhost:8545')
})

it('should correctly identify an actual EthersV5 JsonRpcProvider', () => {
const provider = new JsonRpcProvider('http://localhost:8545')
expect(isEthersV5JsonRpcProvider(provider)).to.be.true
})

it('should return false for an EthersV6 provider', () => {
const provider = new JsonRpcProviderv6('http://localhost:8546')
expect(isEthersV5JsonRpcProvider(provider)).to.be.false
})

it('should return false for a Web3 provider', () => {
const provider = new Web3.providers.HttpProvider('http://localhost:8545')
expect(isEthersV5JsonRpcProvider(provider)).to.be.false
})

it('should return false for a Viem PublicClient', () => {
const provider = createPublicClient({
transport: http('http://localhost:8545'),
})
expect(isEthersV5JsonRpcProvider(provider)).to.be.false
})
})
Copy link
Member

Choose a reason for hiding this comment

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

this set of description doesn't match with what's in the tests

the tests are creating different providers and testing each of them against whether they are ethers v5 provider

whereas the description says
here's an ethers v5 provider, it should return an ethers v5 provider,
here's an ethers v5 provider, (?) should correctly identify it's a v5 provider,
here's an ethers v5 provider, (?) should correctly identify it's not a v6 provider,
here's an ethers v5 provider, (?) should correctly identify it's not a web3 provider

so i'd either change the tests to:

describe('Provider Utilities', () => {
  describe('ethers v5', () => {
    it('should return the URL for an actual EthersV5 provider', () => {
      const provider = new JsonRpcProvider('http://localhost:8545')
      const url = getEthersV5Url(provider)
      expect(url).to.be.equal('http://localhost:8545')
    })

    it('should be correctly identified as an EthersV5 JsonRpcProvider', () => {
      const provider = new JsonRpcProvider('http://localhost:8545')
      expect(isEthersV5JsonRpcProvider(provider)).to.be.true
    })

    it('should not be identified as an EthersV6 provider', () => {
      const provider = new JsonRpcProvider('http://localhost:8545')
      expect(isEthersV6JsonRpcProvider(provider)).to.be.false
    })
   ...
  })

or

describe('Provider Utilities', () => {
  describe('isEthersV5JsonRpcProvider', () => {
    it('should correctly identify an EthersV5 JsonRpcProvider', () => {
      const provider = new JsonRpcProvider('http://localhost:8545')
      const url = getEthersV5Url(provider)
      expect(url).to.be.equal('http://localhost:8545')
      expect(isEthersV5JsonRpcProvider(provider)).to.be.true
    })

    it('should correctly identify an EthersV6 provider is not an EthersV5 provider', () => {
      const provider = new JsonRpcProviderv6('http://localhost:8546')
      expect(isEthersV5JsonRpcProvider(provider)).to.be.false
    })
   ...
  })

something along those lines ^
feel free to figure out what's best

@spsjvc spsjvc marked this pull request as draft June 14, 2024 10:28
@douglance douglance closed this Jun 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants