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

feat: migrate EVM Connector #17

Closed
wants to merge 15 commits into from
Closed

feat: migrate EVM Connector #17

wants to merge 15 commits into from

Conversation

K1-R1
Copy link
Member

@K1-R1 K1-R1 commented Mar 21, 2024

This PR migrates the evm-connector package from it's old repo.
The package has been refactored to achieve the following:

  • Migrate all the code to a self contained package evm-connector.
  • Export the connector on the umbrella package connectors.
  • Ensure all tests are up to date and working with Vitest.

Closes #12

Additional notes and open questions:

  • In order for the evm-connector package's tests to pass when run from the repo's root I had to manually declare the process.env.GENESIS_SECRET = ... within the test file. There is probably a better solution.
  • I have not included this package in the defaultConnectors.
  • pnpm fuels-forc is set to version 0.74.0, this is to use forc 0.50.0. forc 0.51.1 introduces an issue where EvmAddress cannot be initialised as a configureable.

Copy link

vercel bot commented Mar 21, 2024

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

Name Status Preview Comments Updated (UTC)
fuel-connectors ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 29, 2024 3:13am

Copy link
Contributor

github-actions bot commented Mar 21, 2024

Coverage Report for Fuel Wallet (./packages/fuel-wallet)

Status Category Percentage Covered / Total
🔵 Lines 72.29% 261 / 361
🔵 Statements 72.29% 261 / 361
🔵 Functions 53.33% 16 / 30
🔵 Branches 90.47% 19 / 21
File CoverageNo changed files found.
Generated in workflow #63

Copy link
Contributor

github-actions bot commented Mar 21, 2024

Coverage Report for Fuel Development Wallet (./packages/fuel-development-wallet)

Status Category Percentage Covered / Total
🔵 Lines 100% 19 / 19
🔵 Statements 100% 19 / 19
🔵 Functions 100% 1 / 1
🔵 Branches 100% 1 / 1
File CoverageNo changed files found.
Generated in workflow #63

Copy link
Contributor

github-actions bot commented Mar 21, 2024

Coverage Report for Fuelet Wallet (./packages/fuelet-wallet)

Status Category Percentage Covered / Total
🔵 Lines 100% 22 / 22
🔵 Statements 100% 22 / 22
🔵 Functions 100% 1 / 1
🔵 Branches 100% 1 / 1
File CoverageNo changed files found.
Generated in workflow #63

@K1-R1 K1-R1 changed the title Migrate EVM Connector feat: migrate EVM Connector Mar 21, 2024
@K1-R1
Copy link
Member Author

K1-R1 commented Mar 22, 2024

I don't seem to able to commit a changeset; my husky - pre-commit script fails as below when trying to commit just the changeset.md file:
Screenshot 2024-03-22 at 00 24 43

@K1-R1
Copy link
Member Author

K1-R1 commented Mar 22, 2024

I don't seem to able to commit a changeset; my husky - pre-commit script fails as below when trying to commit just the changeset.md file: Screenshot 2024-03-22 at 00 24 43

Resolved by commit 2ed27de

packages/evm-connector/package.json Outdated Show resolved Hide resolved
import path from 'node:path';
import { fileURLToPath } from 'node:url';

export default defineConfig({
Copy link
Member

Choose a reason for hiding this comment

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

it doesn't look we're using any vite app around this PR, did I miss something?
could we delete this file and vite package?

Copy link
Member Author

Choose a reason for hiding this comment

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

Without this file the beforeAll in the test file times out due to __dirname being undefined.
Additionally, as I understand it the following provides access to window:

test: {
    environment: 'jsdom',
  },

Is there another file where I should be doing this instead?

packages/evm-connector/src/EvmWalletConnector.ts Outdated Show resolved Hide resolved
this.predicate = predicates['verification-predicate'];
this.installed = true;
this.config = Object.assign(config, {
fuelProvider: 'https://beta-5.fuel.network/graphql',
Copy link
Member

Choose a reason for hiding this comment

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

should we keep this hard-coded on this package?

Copy link
Member Author

Choose a reason for hiding this comment

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

I have changed this to:

this.config = Object.assign(config, {
      fuelProvider: config.fuelProvider || BETA_5_URL,
      ethProvider: config.ethProvider || window.ethereum,
    });

The reason for the hardcoding was to allow the connector to be instantiated in the same way as the other connectors, with no params needed:

const fuel = new Fuel({
  connectors: [
    new FuelWalletConnector(),
    new EVMWalletConnector(),
  ],
});

In which case the EVMWalletConnector defaults to beta-5.
I'm open to suggestions on if this should be changed

packages/evm-connector/package.json Outdated Show resolved Hide resolved
packages/evm-connector/package.json Outdated Show resolved Hide resolved
packages/evm-connector/package.json Outdated Show resolved Hide resolved
Comment on lines +46 to +47
"vite": "^5.0.10",
"vite-plugin-dts": "^3.6.4",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"vite": "^5.0.10",
"vite-plugin-dts": "^3.6.4",

Copy link
Member Author

Choose a reason for hiding this comment

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

Linked to the above comment about the vite.config.ts file: #17 (comment)

packages/evm-connector/package.json Outdated Show resolved Hide resolved
@luizstacio
Copy link
Member

Closed by #28

@luizstacio luizstacio closed this Apr 3, 2024
@helciofranco helciofranco deleted the K1-R1/migrate-metamask branch April 4, 2024 12:13
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.

Migrate EVM Connector Add Fuel MetaMask Connector on the Fuel Bridge
3 participants