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: Viem migration #887

Closed
wants to merge 94 commits into from
Closed

feat: Viem migration #887

wants to merge 94 commits into from

Conversation

tmjssz
Copy link
Contributor

@tmjssz tmjssz commented Jun 27, 2024

What it solves

Resolves issues #870 and #871.

How this PR fixes it

This pull request migrates the entire repository to viem, removing ethers as a dependency. ethers is retained only as a devDependency for the test-utils in api-kit and protocol-kit.

⚠️ Breaking Changes

protocol-kit

  • SafeProvider
    • getExternalProvider now returns a viem PublicClient (previously an ethers Provider).
    • getExternalSigner now returns a viem WalletClient (previously an ethers AbstractSigner).
    • getTransaction now returns a viem Transaction (previously an ethers TransactionResponse).
    • encodeParameters and decodeParameters now expect a string as the first parameter, containing the Human-readable ABI parameters (previously a string[]).
  • All ABI parameters are now a viem Abi (previously JsonFragment | JsonFragment[]). This affects:
    • defaultAbi and customContractAbi in all Contract classes.
    • customContractAbi parameter in all get...ContractInstance functions and in the GetContractProps type.
  • getSafeProxyFactoryContractInstance function's signerOrProvider parameter is now a viem PublicClient.

api-kit

  • SafeApiKit's addSafeDelegate and removeSafeDelegate functions now expect a viem WalletClient as the signer prop (previously an ethers Signer).

relay-kit

  • Safe4337Options now expects a bundlerClient prop of viem's type PublicClient (previously ethers JsonRpcProvider).

auth-kit

  • AuthKitEthereumProvider type is now a viem EIP1193Provider (previously ethers Eip1193Provider). This new type restricts the RPC methods for EIP-1193, as opposed to the ethers type, which accepted any string as a method name.

@coveralls
Copy link

coveralls commented Jun 27, 2024

Pull Request Test Coverage Report for Build 9697300982

Details

  • 53 of 56 (94.64%) changed or added relevant lines in 8 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.08%) to 77.786%

Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/relay-kit/src/packs/safe-4337/utils.ts 8 11 72.73%
Totals Coverage Status
Change from base Build 9659157719: 0.08%
Covered Lines: 768
Relevant Lines: 918

💛 - Coveralls

@tmjssz tmjssz marked this pull request as draft July 1, 2024 13:52
@tmjssz tmjssz force-pushed the feat/viem-migration branch from d69e1d4 to 7e0bd8e Compare July 2, 2024 12:34
@coveralls
Copy link

coveralls commented Jul 2, 2024

Pull Request Test Coverage Report for Build 9761038587

Details

  • 53 of 57 (92.98%) changed or added relevant lines in 8 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.08%) to 77.496%

Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/relay-kit/src/packs/safe-4337/SafeOperation.ts 2 3 66.67%
packages/relay-kit/src/packs/safe-4337/utils.ts 8 11 72.73%
Totals Coverage Status
Change from base Build 9742471451: 0.08%
Covered Lines: 780
Relevant Lines: 935

💛 - Coveralls

@coveralls
Copy link

coveralls commented Jul 2, 2024

Pull Request Test Coverage Report for Build 9761061066

Details

  • 53 of 57 (92.98%) changed or added relevant lines in 8 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.08%) to 77.496%

Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/relay-kit/src/packs/safe-4337/SafeOperation.ts 2 3 66.67%
packages/relay-kit/src/packs/safe-4337/utils.ts 8 11 72.73%
Totals Coverage Status
Change from base Build 9742471451: 0.08%
Covered Lines: 780
Relevant Lines: 935

💛 - Coveralls

To avoid breaking changes we keep regular `string` types for now and do type casting whenever necessary for calling viem's functions.
@coveralls
Copy link

coveralls commented Jul 2, 2024

Pull Request Test Coverage Report for Build 9762097670

Details

  • 48 of 52 (92.31%) changed or added relevant lines in 8 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.08%) to 77.496%

Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/relay-kit/src/packs/safe-4337/SafeOperation.ts 1 2 50.0%
packages/relay-kit/src/packs/safe-4337/utils.ts 7 10 70.0%
Totals Coverage Status
Change from base Build 9742471451: 0.08%
Covered Lines: 780
Relevant Lines: 935

💛 - Coveralls

@coveralls
Copy link

coveralls commented Jul 4, 2024

Pull Request Test Coverage Report for Build 9792499339

Details

  • 71 of 75 (94.67%) changed or added relevant lines in 13 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.06%) to 77.479%

Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/relay-kit/src/packs/safe-4337/SafeOperation.ts 1 2 50.0%
packages/relay-kit/src/packs/safe-4337/utils.ts 7 10 70.0%
Totals Coverage Status
Change from base Build 9742471451: 0.06%
Covered Lines: 779
Relevant Lines: 934

💛 - Coveralls

@coveralls
Copy link

coveralls commented Jul 5, 2024

Pull Request Test Coverage Report for Build 9806031807

Details

  • 71 of 75 (94.67%) changed or added relevant lines in 13 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.06%) to 77.479%

Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/relay-kit/src/packs/safe-4337/SafeOperation.ts 1 2 50.0%
packages/relay-kit/src/packs/safe-4337/utils.ts 7 10 70.0%
Totals Coverage Status
Change from base Build 9742471451: 0.06%
Covered Lines: 779
Relevant Lines: 934

💛 - Coveralls

@coveralls
Copy link

coveralls commented Jul 5, 2024

Pull Request Test Coverage Report for Build 9807577285

Details

  • 71 of 75 (94.67%) changed or added relevant lines in 13 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.06%) to 77.479%

Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/relay-kit/src/packs/safe-4337/SafeOperation.ts 1 2 50.0%
packages/relay-kit/src/packs/safe-4337/utils.ts 7 10 70.0%
Totals Coverage Status
Change from base Build 9807507022: 0.06%
Covered Lines: 779
Relevant Lines: 934

💛 - Coveralls

@coveralls
Copy link

coveralls commented Jul 5, 2024

Pull Request Test Coverage Report for Build 9810052440

Details

  • 71 of 75 (94.67%) changed or added relevant lines in 13 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.08%) to 74.541%

Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/relay-kit/src/packs/safe-4337/SafeOperation.ts 1 2 50.0%
packages/relay-kit/src/packs/safe-4337/utils.ts 7 10 70.0%
Totals Coverage Status
Change from base Build 9809678697: 0.08%
Covered Lines: 791
Relevant Lines: 989

💛 - Coveralls

leonardotc and others added 6 commits July 26, 2024 09:45
* Extend SafeProvider's external client type to `PublicClient | WalletClient`

* Introduce `ExternalClient` type alias and use it as type for `#externalProvider` in SafeProvider

* With the extended type we need to use viem methods individually from `viem/actions` instead of calling them directly on the client itself

* Extend SafeProvider by a `readContract` method to fix TS error in Safe4337Pack

Also fix unit test for Safe4337Pack

* Fix custom chain support

---------

Co-authored-by: leonardotc <[email protected]>
} from 'viem'
import { asHex } from '../types'

// This whole file was copied (and softly adapted) from viem in order to expose the function that provides just the encoding.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I saw this was done to avoid to break the types in the current iteration. This was not explicitly explained here, and that this piece of code should be removed as soon as possible. Please add some comment signaling this.

@dasanra dasanra linked an issue Aug 1, 2024 that may be closed by this pull request
@dasanra dasanra force-pushed the feat/viem-migration branch from 1c7dfb4 to 4dc89f1 Compare August 8, 2024 15:03
@dasanra dasanra closed this Sep 11, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Sep 11, 2024
@dasanra dasanra deleted the feat/viem-migration branch October 2, 2024 13:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[SDK Refactoring] Testing playground and demo apps. [SDK Refactoring] Update SDK tests to use viem
5 participants