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

Remove Ethers dependency from sapphire-paratime package, add viem v2 & wagmi v2 packages #303

Open
wants to merge 58 commits into
base: main
Choose a base branch
from

Conversation

CedarMist
Copy link
Member

@CedarMist CedarMist commented Apr 24, 2024

  • Simplified sapphire-paratime dependencies

    • Removed ethers, this reduces bundled size significantly when used standalone or with Viem
    • Removed tweetnacl.js, extracted necessary functions into munacl.ts
    • Removed generic sapphire.wrap function, it now exposes wrapEthereumProvider which only wraps EIP-1193 compatible providers
  • Tests:

    • Removed many mock based tests, instead relying more heavily on integration tests to verify actual functionality
    • All CI tests run through a local RPC proxy (clients/js/scripts/proxy.ts) which will die if any unencrypted transactions, calls or gas estimates are made.
      • This ensures that the clients & integrations apply encryption consistently, and will prevent any regressions where encryption is accidentally broken.
  • Integrations:

    • Each SDK has its own integration package + tests
      • @oasisprotocol/sapphire-ethers-v6
      • @oasisprotocol/wagmi-v2
      • @oasisprotocol/viem-v2
  • Removed duplicate example projects (e.g. where two projects use the exact same underlying libraries)

    • Each example project also serves as an integration test
      • hardhat-viem (Hardhat + Viem)
      • onchain-signer (Hardhat + Ignition + Ethers v6)
      • hardhat-boilerplate (Hardhat + Ethers v5 + frontend & bundling)
      • wagmi-v2 (Wagmi + frontend & bundling)
      • wagmi-v1

Copy link

netlify bot commented Apr 24, 2024

Deploy Preview for oasisprotocol-sapphire-paratime canceled.

Name Link
🔨 Latest commit 5125cf5
🔍 Latest deploy log https://app.netlify.com/sites/oasisprotocol-sapphire-paratime/deploys/667d40803c5be20008d95d53

@CedarMist CedarMist self-assigned this Apr 30, 2024
@CedarMist CedarMist added p:1 Priority: high dependencies Pull requests that update a dependency file js Pull requests that update JS code client docs Documentation javascript Pull requests that update Javascript code labels Apr 30, 2024
@CedarMist CedarMist marked this pull request as ready for review April 30, 2024 09:41
@aefhm aefhm self-requested a review April 30, 2024 19:19
@CedarMist
Copy link
Member Author

CedarMist commented May 10, 2024

Feedback from Matevz

  • use @noble/curves rather than munacl.ts
  • need to double check that multiple signers in hardhat with ethers work
  • does walletconnect & rainbowkit etc, work on mobile?
  • wagmi v2 example encrypts the deployment transaction
    • we want unencrypted deployments, always?
  • pure viem-v2 example in integrations
  • tag current branch as 1.x / stable
  • new stuff goes as 2.x / beta?

@CedarMist CedarMist changed the title Cedar mist/client refactor Remove Ethers dependency from sapphire-paratime package, add viem v2 & wagmi v2 packages May 10, 2024
Copy link
Contributor

@aefhm aefhm left a comment

Choose a reason for hiding this comment

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

Comments cover client code. Will send notes on integrations next. Still need to QA as well.

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
clients/js/CHANGELOG.md Outdated Show resolved Hide resolved
clients/js/README.md Outdated Show resolved Hide resolved
clients/js/package.json Show resolved Hide resolved
clients/js/src/provider.ts Show resolved Hide resolved
clients/js/src/provider.ts Outdated Show resolved Hide resolved
clients/js/scripts/proxy.ts Outdated Show resolved Hide resolved
clients/js/src/cipher.ts Show resolved Hide resolved
clients/js/test/cipher.spec.ts Show resolved Hide resolved
@matevz
Copy link
Member

matevz commented May 13, 2024

* tag current branch as 1.x / stable
* new stuff goes as 2.x / beta?

Sleeping over this, I would name the stable branch as clients/js/1.3.x and keep the new refactored codebase inside the main branch. If we find anything worth fixing in the stable branch as well, then we backport the fix from main to clients/js/1.3.x. If the fix is solely for 1.3.x, then we just fix the issue on the branch directly.

Copy link
Contributor

@aefhm aefhm left a comment

Choose a reason for hiding this comment

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

I took a QA pass around examples and the Hardhat fork which worked great :)

"types": "./dist/_types/index.d.ts",
"default": "./dist/_esm/index.js"
},
"scripts": {
Copy link
Contributor

Choose a reason for hiding this comment

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

(non-blocking) noting we don't have tests for this one.

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't have automated tests for this.

Will need Synpress / DappWrite tests for the frontend related stuff as can't test Wagmi + MetaMask without that.

clients/Makefile Outdated Show resolved Hide resolved
clients/js/package.json Show resolved Hide resolved
clients/js/scripts/proxy.ts Show resolved Hide resolved
.github/workflows/contracts-test.yaml Outdated Show resolved Hide resolved
clients/js/src/calldatapublickey.ts Outdated Show resolved Hide resolved
clients/js/tsconfig.cjs.json Show resolved Hide resolved
examples/hardhat-viem/package.json Outdated Show resolved Hide resolved
examples/hardhat-viem/tsconfig.json Outdated Show resolved Hide resolved
integrations/ethers-v6/.gitignore Outdated Show resolved Hide resolved
request,
),
request,
// TODO: wrap `getSigner()`
Copy link
Contributor

Choose a reason for hiding this comment

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

Or I guess devs could do this themselves.

Copy link
Member Author

Choose a reason for hiding this comment

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

So, I can't easily test this without a provider like MetaMask, as getSigner will try to enumerate accounts, e.g. in geth.

Copy link
Contributor

@aefhm aefhm left a comment

Choose a reason for hiding this comment

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

I vote to merge, and we can iterate from there.

@aefhm aefhm mentioned this pull request Jun 25, 2024
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client dependencies Pull requests that update a dependency file docs Documentation javascript Pull requests that update Javascript code js Pull requests that update JS code p:1 Priority: high
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants