Migrate From ContractKit to Viem #294

aaronmgdr · 2024-09-25T13:30:20Z

Description

Rather than use ContractKit use viem as the rpc caller / contracts library.

Changes

WalletSigner now takes a sign191 function instead of a contractKit instance. this makes it easy to use with whatever library you like such as viem client.signMessage or ethers.signMessage as long as they support eip191.
Many places that were expecting an address but typed as a regular string are now typed as 0x{string} aka Address
almost everywhere kit has been replaced with client and in some cases walletClient
@celo/phone-number-privacy-common/lib/contracts contract instances used internally (viem getContract + abi + address) getAccountsContract, getCUSDContract, getOdisPaymentsContract,
ContractKit is still needed for the @celo/identity/lib/offchain functions however these are not used in social connect as such ck is now a peer dependencies and marked as optional. Pottentially these offchain functions can be either deprecated or moved to a different codebase.

Tested

Needs someone who understand the system really well to really test it out.

TODO

need to re mock the getDataEncryptionKey method in the authentication.test in common package. one was done but the rest still need it and each seems to require different implementatation
try to replace contractkit with @celo/[email protected]

dont merge until we get this deployed to staging.

Related issues

Fixes Replace contractkit with viem #296

Backwards compatibility

No. This is a Major Change

Documentation

documented in changesets the differences

changeset-bot · 2024-09-25T13:30:24Z

🦋 Changeset detected

Latest commit: cfde4ce

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 7 packages

Name	Type
@celo/phone-number-privacy-monitor	Major
@celo/phone-number-privacy-combiner	Major
@celo/identity	Major
@celo/encrypted-backup	Patch
@celo/phone-number-privacy-common	Major
@celo/phone-number-privacy-signer	Major
odis-example-scripts	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

gitguardian · 2024-09-25T13:30:25Z

⚠️ GitGuardian has uncovered 1 secret following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

🔎 Detected hardcoded secret in your pull request

GitGuardian id	GitGuardian status	Secret	Commit	Filename
10990680	Triggered	Generic High Entropy Secret	`4a7b2c8`	packages/common/src/test/values.ts	View secret

🛠 Guidelines to remediate hardcoded secrets

Understand the implications of revoking this secret by investigating where it is used in your code.
Replace and store your secret safely. Learn here the best practices.
Revoke and rotate this secret.
If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider

following these best practices for managing and storing secrets including API keys and other credentials
install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.

^{🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.}

socket-security · 2024-09-25T13:31:40Z

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package	New capabilities	Transitives	Size	Publisher
npm/@celo/[email protected]	network	`+2`	5.2 MB	app-tooling
npm/@celo/[email protected]	Transitive: environment, filesystem, network	`+16`	45.3 MB	app-tooling
npm/@celo/[email protected]	Transitive: environment, filesystem, network, shell	`+46`	32.3 MB	app-tooling
npm/@celo/[email protected]	Transitive: environment, eval, filesystem, network, unsafe	`+145`	9.4 MB	app-tooling
npm/[email protected]	network	`0`	12.8 MB	jmoxey

🚮 Removed packages: npm/@celo/[email protected]

View full report↗︎

aaronmgdr · 2024-09-25T13:32:36Z

packages/common/src/test/utils.ts

-  await selfTransferTx.sendAndWaitForReceipt({ from: account })
-}
-
-export async function registerWalletAddress(


a test function not used in any tests

fix rawKey should actually not be a hex

alecps · 2025-01-28T22:12:51Z

README.md

@@ -79,7 +80,7 @@ The following steps use the Celo [ContractKit](https://docs.celo.org/developer/c
    // authSigner provides information needed to authenticate with ODIS
    const authSigner: AuthSigner = {
        authenticationMethod: OdisUtils.Query.AuthenticationMethod.WALLET_KEY,
-        contractKit: kit,
+        sign191: ({message, account}) => viemClient.signMessage({message, account}),


Nit: does it make sense to call this sign191 instead of just sign

alecps · 2025-01-28T22:16:54Z

apps/signer/README.md

@@ -52,6 +52,7 @@ This could be a node with RPC set up. Preferably this would be an node dedicated

 - `BLOCKCHAIN_PROVIDER` - The blockchain node provider for chain state access. `
 - `BLOCKCHAIN_API_KEY` - Optional API key to be added to the authentication header. `
+- `CHAIN_ID` should be 44220 or celo or 44787 on alfajores


Suggested change

- `CHAIN_ID` should be 44220 or celo or 44787 on alfajores

- `CHAIN_ID` should be 44220 for Celo Mainnet or 44787 for Celo Alfajores

alecps · 2025-01-28T22:17:44Z

apps/signer/.env

@@ -3,6 +3,7 @@ NODE_ENV=development
 #SERVER_SSL_KEY_PATH=./server.key
 #SERVER_SSL_CERT_PATH=./server.cert
 BLOCKCHAIN_PROVIDER=https://alfajores-forno.celo-testnet.org
+CHAIN_ID=42220


Shouldn't this be 44787 for alfajores to be consistent with the RPC url?

alecps · 2025-01-28T22:28:21Z

apps/monitor/src/index.ts

-  async () => testPNPSignQuery(blockchainProvider.value(), contextName.value() as any),
+  async () =>
+    testPNPSignQuery(
+      { rpcURL: blockchainProvider.value(), chainID: chainID.value() as 44787 },


Why are we casting this to a specific chain ID? What if we want to run the test on mainnet?

+1 it is in fact running for both networks.

alecps · 2025-01-28T22:39:10Z

apps/combiner/src/server.ts

  const logger = rootLogger(config.serviceName)

-  kit = kit ?? getContractKitWithAgent(config.blockchain)
+  viemClient = viemClient ?? getWalletClientWithAgent(config.blockchain)


Nit: consider naming all these "ClientAccountService" etc. to be consistent with the Signer server.ts file

alecps · 2025-01-28T22:42:32Z

apps/combiner/test/end-to-end/resources.ts

-contractKit.defaultAccount = ACCOUNT_ADDRESS
+export const client = createWalletClient({
+  transport: http(DEFAULT_FORNO_URL),
+  chain: celo,


I think this needs to be configurable to be either celo or celoAlfajores

+1 Perhaps getting it from the env file as well.

alecps · 2025-01-28T22:49:55Z

@aaronmgdr Better late than never 😅. I haven't quite made it through the whole PR but have made a big dent and will continue to chip away at it. In general it's looking great so far. Thanks again for taking the time to do this and apologies it's taken me so long to get around to reviewing it

alecps · 2025-01-28T22:57:48Z

packages/common/src/utils/authentication.ts

-export function newContractKitFetcher(
-  contractKit: ContractKit,
+export function newDEKFetcher(
+  viemClient: Client,


Nit: consider naming this walletClient for consistency

soloseng

Sorry, this just got flagged to me.

Thanks for taking the time to work on this.

Looks good. Not sure where this sits on your priority list as of now, but made a few comments.

soloseng · 2025-02-04T20:55:39Z

apps/combiner/test/end-to-end/resources.ts

-contractKit.defaultAccount = ACCOUNT_ADDRESS
+export const client = createWalletClient({
+  transport: http(DEFAULT_FORNO_URL),
+  chain: celo,


+1 Perhaps getting it from the env file as well.

soloseng · 2025-02-04T21:50:57Z

apps/monitor/src/index.ts

-  async () => testPNPSignQuery(blockchainProvider.value(), contextName.value() as any),
+  async () =>
+    testPNPSignQuery(
+      { rpcURL: blockchainProvider.value(), chainID: chainID.value() as 44787 },


+1 it is in fact running for both networks.

soloseng · 2025-02-04T22:02:15Z

apps/monitor/src/query.ts

+  let accountAddress: Address
+
+  if (useDEK) {
+    // im not sure why this is like this


This is to randomize which private key is used. It's particularly usefull during load testing when we want to have different type of request being sent.

soloseng · 2025-02-05T16:12:01Z

apps/signer/test/end-to-end/pnp.test.ts

@@ -28,31 +35,35 @@ const {
  PHONE_NUMBER,
  PRIVATE_KEY1,
  PRIVATE_KEY2,
-  PRIVATE_KEY3,
+  // PRIVATE_KEY3,


can be removed if unused.

soloseng · 2025-02-05T16:12:08Z

apps/signer/test/end-to-end/pnp.test.ts

-kit.addAccount(PRIVATE_KEY3)
+const account1 = privateKeyToAccount(ensureLeading0x(PRIVATE_KEY1))
+const account2 = privateKeyToAccount(ensureLeading0x(PRIVATE_KEY2))
+// const account3 = privateKeyToAccount(ensureLeading0x(PRIVATE_KEY3))


soloseng · 2025-02-05T16:30:31Z

packages/common/test/utils/authentication.test.ts


-describe('Authentication test suite', () => {
+describe.skip('Authentication test suite', () => {


is this skip intended to stay?

soloseng · 2025-02-05T16:31:55Z

packages/common/test/utils/authentication.test.ts

-        },
-      } as ContractKit
-      const dekFetcher = newContractKitFetcher(mockContractKit, logger)
+      // const mockContractKit = {


remove commented code

soloseng · 2025-02-05T16:34:09Z

packages/common/test/utils/authentication.test.ts

+      //     getAccounts: async () => {
+      //       return Promise.resolve({
+      //         getDataEncryptionKey: async (_: string) => {
+      //           return 'notAValidKeyEncryption'


Noticed that the return value is different with each mockContractKit. Is that reflected with the viem client?

soloseng · 2025-02-05T16:35:15Z

packages/common/test/utils/authentication.test.ts

+      //         getDataEncryptionKey: async (_: string) => {
+      //           // NOTE: elliptic is disabled elsewhere in this library to prevent
+      //           // accidental signing of truncated messages.
+      //           const EC = require('elliptic').ec


soloseng · 2025-02-05T16:37:15Z

packages/encrypted-backup/src/utils.ts

 import * as crypto from 'crypto'
 import { ComputationalHardeningConfig, ComputationalHardeningFunction } from './config'
 import { DecryptionError, EncryptionError, PbkdfError, ScryptError } from './errors'

 // NOTE: This module is intended for use within the @celo/encrypted-backup package and so is not
 // exported in the index.ts file.

-/** Pared down ReadOnlyWallet type that supports the required functions of EIP-712 signing. */
-export type EIP712Wallet = Pick<ReadOnlyWallet, 'getAccounts' | 'hasAccount' | 'signTypedData'>
+/** Pared down ReadOnlyWallet type that supports the required functions of EIP-712 signing. */ export interface EIP712Wallet {


move code to next line to improve readability

aaronmgdr added 4 commits August 29, 2024 13:34

WIP use viem instead of contractkit

4a7b2c8

add missing parts

d012ef3

first final draft

d2091b6

instead of a viem client instance just use a sign function more generic.

838939b

aaronmgdr commented Sep 25, 2024

View reviewed changes

alecps requested review from viral-sangani, therealharpaljadeja and alecps September 30, 2024 18:46

aaronmgdr and others added 7 commits October 11, 2024 17:03

convert contractkit mocks to mocks for our viem contract accessors,

4a09193

fix rawKey should actually not be a hex

add change sets for packages

cc5df33

Fix up configs,

6dada95

fix config

49f43be

remock the getDataEncryptionKey

0f4bfe1

skip auth tests for now

1f8fb50

upgrade celo dependencies

cfde4ce

alecps reviewed Jan 28, 2025

View reviewed changes

alecps requested review from soloseng and removed request for viral-sangani and therealharpaljadeja January 28, 2025 22:50

alecps reviewed Jan 28, 2025

View reviewed changes

soloseng reviewed Feb 5, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate From ContractKit to Viem #294

Migrate From ContractKit to Viem #294

aaronmgdr commented Sep 25, 2024 •

edited

Loading

changeset-bot bot commented Sep 25, 2024 •

edited

Loading

gitguardian bot commented Sep 25, 2024 •

edited

Loading

socket-security bot commented Sep 25, 2024 •

edited

Loading

aaronmgdr Sep 25, 2024

alecps Jan 28, 2025

alecps Jan 28, 2025

alecps Jan 28, 2025

alecps Jan 28, 2025

soloseng Feb 4, 2025

alecps Jan 28, 2025

alecps Jan 28, 2025

soloseng Feb 4, 2025

alecps commented Jan 28, 2025

alecps Jan 28, 2025 •

edited

Loading

soloseng left a comment

soloseng Feb 4, 2025

soloseng Feb 4, 2025

soloseng Feb 4, 2025

soloseng Feb 5, 2025

soloseng Feb 5, 2025

soloseng Feb 5, 2025

soloseng Feb 5, 2025

soloseng Feb 5, 2025

soloseng Feb 5, 2025

soloseng Feb 5, 2025

	- `CHAIN_ID` should be 44220 or celo or 44787 on alfajores
	- `CHAIN_ID` should be 44220 for Celo Mainnet or 44787 for Celo Alfajores


		describe('Authentication test suite', () => {
		describe.skip('Authentication test suite', () => {

Migrate From ContractKit to Viem #294

Are you sure you want to change the base?

Migrate From ContractKit to Viem #294

Conversation

aaronmgdr commented Sep 25, 2024 • edited Loading

Description

Changes

Tested

TODO

Related issues

Backwards compatibility

Documentation

changeset-bot bot commented Sep 25, 2024 • edited Loading

🦋 Changeset detected

gitguardian bot commented Sep 25, 2024 • edited Loading

⚠️ GitGuardian has uncovered 1 secret following the scan of your pull request.

socket-security bot commented Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alecps commented Jan 28, 2025

alecps Jan 28, 2025 • edited Loading

Choose a reason for hiding this comment

soloseng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronmgdr commented Sep 25, 2024 •

edited

Loading

changeset-bot bot commented Sep 25, 2024 •

edited

Loading

gitguardian bot commented Sep 25, 2024 •

edited

Loading

socket-security bot commented Sep 25, 2024 •

edited

Loading

alecps Jan 28, 2025 •

edited

Loading