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: upgrade viem to v2 #2284

Merged
merged 12 commits into from
Feb 23, 2024
Merged

feat: upgrade viem to v2 #2284

merged 12 commits into from
Feb 23, 2024

Conversation

tash-2s
Copy link
Contributor

@tash-2s tash-2s commented Feb 20, 2024

closes #2267

Copy link

changeset-bot bot commented Feb 20, 2024

🦋 Changeset detected

Latest commit: 39af8bb

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

This PR includes changesets to release 30 packages
Name Type
@latticexyz/block-logs-stream Major
@latticexyz/cli Major
@latticexyz/common Major
@latticexyz/config Major
@latticexyz/dev-tools Major
@latticexyz/faucet Major
@latticexyz/protocol-parser Major
@latticexyz/schema-type Major
@latticexyz/store-indexer Major
@latticexyz/store-sync Major
@latticexyz/store Major
@latticexyz/world Major
create-mud Major
@latticexyz/world-modules Major
@latticexyz/recs Major
@latticexyz/react Major
@latticexyz/abi-ts Major
@latticexyz/ecs-browser Major
@latticexyz/gas-report Major
@latticexyz/network Major
@latticexyz/noise Major
@latticexyz/phaserx Major
@latticexyz/services Major
@latticexyz/solecs Major
solhint-config-mud Major
solhint-plugin-mud Major
@latticexyz/std-client Major
@latticexyz/std-contracts Major
@latticexyz/store-cache Major
@latticexyz/utils Major

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

@tash-2s
Copy link
Contributor Author

tash-2s commented Feb 20, 2024

warning when executing pnpm install at the repository root:

 WARN  Issues with peer dependencies found
packages/cli
└─┬ viem 2.7.11
  └─┬ abitype 1.0.0
    └── ✕ unmet peer zod@"^3 >=3.22.0": found 3.21.4

packages/config
└─┬ viem 2.7.11
  └─┬ abitype 1.0.0
    └── ✕ unmet peer zod@"^3 >=3.22.0": found 3.21.4

packages/faucet
└─┬ viem 2.7.11
  └─┬ abitype 1.0.0
    └── ✕ unmet peer zod@"^3 >=3.22.0": found 3.21.4

packages/react
└─┬ @testing-library/react-hooks 8.0.1
  ├── ✕ unmet peer @types/react@"^16.9.0 || ^17.0.0": found 18.2.22
  ├── ✕ unmet peer react@"^16.9.0 || ^17.0.0": found 18.2.0
  └── ✕ unmet peer react-test-renderer@"^16.9.0 || ^17.0.0": found 18.2.0

packages/store
└─┬ viem 2.7.11
  └─┬ abitype 1.0.0
    └── ✕ unmet peer zod@"^3 >=3.22.0": found 3.21.4

packages/store-indexer
└─┬ viem 2.7.11
  └─┬ abitype 1.0.0
    └── ✕ unmet peer zod@"^3 >=3.22.0": found 3.21.4

packages/store-sync
└─┬ viem 2.7.11
  └─┬ abitype 1.0.0
    └── ✕ unmet peer zod@"^3 >=3.22.0": found 3.21.4

packages/world
└─┬ viem 2.7.11
  └─┬ abitype 1.0.0
    └── ✕ unmet peer zod@"^3 >=3.22.0": found 3.21.4

@holic
Copy link
Member

holic commented Feb 20, 2024

I think we could bump zod here too! At least to the minimum range expected by viem/abitype.

I wouldn't worry about the warnings for React-related packages, can solve that separately.

@tash-2s
Copy link
Contributor Author

tash-2s commented Feb 20, 2024

@holic The abitype's zod peer dependency is optional, and we're not using abitype/zod, so I believe it doesn't cause an issue. But it's better to resolve the warning later.


Correction: viem uses abitype but doesn't use abitype/zod (so the warning shouldn't be an actual issue)

@tash-2s
Copy link
Contributor Author

tash-2s commented Feb 20, 2024

Also, I might be able to pass the typescript test without upgrading the mud's abitype, but also it's better to upgrade it alongside viem.

@holic
Copy link
Member

holic commented Feb 20, 2024

but also it's better to upgrade it alongside viem

agree on aligning our abitype version with the one that viem's version uses

@@ -9,6 +9,7 @@
"isolatedModules": true,
"esModuleInterop": true,
"forceConsistentCasingInFileNames": true,
"skipLibCheck": true,
Copy link
Member

Choose a reason for hiding this comment

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

what are the consequences of this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By leaving skipLibCheck at default false, I encounter the following error:

cd packages/block-logs-stream && pnpm tsc --noEmit

../../node_modules/.pnpm/[email protected][email protected][email protected]/node_modules/viem/_types/utils/formatters/transaction.d.ts(51,217): error TS2536: Type 'K_1' cannot be used to index type '{ gas: bigint; value: bigint; blockHash: `0x${string}` | null; blockNumber: bigint | null; from: `0x${string}`; hash: `0x${string}`; input: `0x${string}`; nonce: number; r: `0x${string}`; s: `0x${string}`; to: `0x${string}` | null; ... 11 more ...; type: "legacy"; }'.
../../node_modules/.pnpm/[email protected][email protected][email protected]/node_modules/viem/_types/utils/formatters/transaction.d.ts(75,82): error TS2536: Type 'K' cannot be used to index type '{ [K_1 in keyof { gas: bigint; value: bigint; blockHash: `0x${string}` | null; blockNumber: bigint | null; from: `0x${string}`; hash: `0x${string}`; input: `0x${string}`; nonce: number; r: `0x${string}`; s: `0x${string}`; to: `0x${string}` | null; ... 11 more ...; type: "legacy"; } as K_1 extends keyof TOverrideRetu...'.
../../node_modules/.pnpm/[email protected][email protected][email protected]/node_modules/viem/_types/utils/formatters/transaction.d.ts(99,217): error TS2536: Type 'K_1' cannot be used to index type '{ gas: bigint; value: bigint; blockHash: `0x${string}` | null; blockNumber: bigint | null; from: `0x${string}`; hash: `0x${string}`; input: `0x${string}`; nonce: number; r: `0x${string}`; s: `0x${string}`; to: `0x${string}` | null; ... 11 more ...; type: "legacy"; }'.
../../node_modules/.pnpm/[email protected][email protected][email protected]/node_modules/viem/_types/utils/formatters/transaction.d.ts(147,217): error TS2536: Type 'K_3' cannot be used to index type '{ blockHash: `0x${string}` | null; blockNumber: bigint | null; from: `0x${string}`; gas: bigint; hash: `0x${string}`; input: `0x${string}`; nonce: number; r: `0x${string}`; s: `0x${string}`; to: `0x${string}` | null; transactionIndex: number | null; ... 11 more ...; type: "eip2930"; }'.
../../node_modules/.pnpm/[email protected][email protected][email protected]/node_modules/viem/_types/utils/formatters/transaction.d.ts(171,88): error TS2536: Type 'K_2' cannot be used to index type '{ [K_3 in keyof { blockHash: `0x${string}` | null; blockNumber: bigint | null; from: `0x${string}`; gas: bigint; hash: `0x${string}`; input: `0x${string}`; nonce: number; r: `0x${string}`; s: `0x${string}`; to: `0x${string}` | null; transactionIndex: number | null; ... 11 more ...; type: "eip2930"; } as K_3 extends ...'.
../../node_modules/.pnpm/[email protected][email protected][email protected]/node_modules/viem/_types/utils/formatters/transaction.d.ts(195,217): error TS2536: Type 'K_3' cannot be used to index type '{ blockHash: `0x${string}` | null; blockNumber: bigint | null; from: `0x${string}`; gas: bigint; hash: `0x${string}`; input: `0x${string}`; nonce: number; r: `0x${string}`; s: `0x${string}`; to: `0x${string}` | null; transactionIndex: number | null; ... 11 more ...; type: "eip2930"; }'.
../../node_modules/.pnpm/[email protected][email protected][email protected]/node_modules/viem/_types/utils/formatters/transaction.d.ts(243,217): error TS2536: Type 'K_5' cannot be used to index type '{ blockHash: `0x${string}` | null; blockNumber: bigint | null; from: `0x${string}`; gas: bigint; hash: `0x${string}`; input: `0x${string}`; nonce: number; r: `0x${string}`; s: `0x${string}`; to: `0x${string}` | null; transactionIndex: number | null; ... 11 more ...; type: "eip1559"; }'.
../../node_modules/.pnpm/[email protected][email protected][email protected]/node_modules/viem/_types/utils/formatters/transaction.d.ts(267,88): error TS2536: Type 'K_4' cannot be used to index type '{ [K_5 in keyof { blockHash: `0x${string}` | null; blockNumber: bigint | null; from: `0x${string}`; gas: bigint; hash: `0x${string}`; input: `0x${string}`; nonce: number; r: `0x${string}`; s: `0x${string}`; to: `0x${string}` | null; transactionIndex: number | null; ... 11 more ...; type: "eip1559"; } as K_5 extends ...'.
../../node_modules/.pnpm/[email protected][email protected][email protected]/node_modules/viem/_types/utils/formatters/transaction.d.ts(291,217): error TS2536: Type 'K_5' cannot be used to index type '{ blockHash: `0x${string}` | null; blockNumber: bigint | null; from: `0x${string}`; gas: bigint; hash: `0x${string}`; input: `0x${string}`; nonce: number; r: `0x${string}`; s: `0x${string}`; to: `0x${string}` | null; transactionIndex: number | null; ... 11 more ...; type: "eip1559"; }'.
../../node_modules/.pnpm/[email protected][email protected][email protected]/node_modules/viem/_types/utils/formatters/transaction.d.ts(339,219): error TS2536: Type 'K_7' cannot be used to index type '{ blockHash: `0x${string}` | null; blockNumber: bigint | null; from: `0x${string}`; gas: bigint; hash: `0x${string}`; input: `0x${string}`; nonce: number; r: `0x${string}`; s: `0x${string}`; to: `0x${string}` | null; transactionIndex: number | null; ... 11 more ...; type: "eip4844"; }'.
../../node_modules/.pnpm/[email protected][email protected][email protected]/node_modules/viem/_types/utils/formatters/transaction.d.ts(363,88): error TS2536: Type 'K_6' cannot be used to index type '{ [K_7 in keyof { blockHash: `0x${string}` | null; blockNumber: bigint | null; from: `0x${string}`; gas: bigint; hash: `0x${string}`; input: `0x${string}`; nonce: number; r: `0x${string}`; s: `0x${string}`; to: `0x${string}` | null; transactionIndex: number | null; ... 11 more ...; type: "eip4844"; } as K_7 extends ...'.
../../node_modules/.pnpm/[email protected][email protected][email protected]/node_modules/viem/_types/utils/formatters/transaction.d.ts(387,219): error TS2536: Type 'K_7' cannot be used to index type '{ blockHash: `0x${string}` | null; blockNumber: bigint | null; from: `0x${string}`; gas: bigint; hash: `0x${string}`; input: `0x${string}`; nonce: number; r: `0x${string}`; s: `0x${string}`; to: `0x${string}` | null; transactionIndex: number | null; ... 11 more ...; type: "eip4844"; }'.
../../node_modules/.pnpm/[email protected][email protected][email protected]/node_modules/viem/_types/utils/formatters/transactionRequest.d.ts(21,598): error TS2536: Type 'K' cannot be used to index type '{ [K_1 in keyof TransactionRequestLegacy<`0x${string}`, `0x${string}`, "0x0"> as K_1 extends keyof TOverrideReturnType ? TOverrideReturnType[K_1] extends void ? never : K_1 : K_1]: K_1 extends keyof TOverrideReturnType ? TOverrideReturnType[K_1] : TransactionRequestLegacy<...>[K_1]; } & TOverrideReturnType'.
../../node_modules/.pnpm/[email protected][email protected][email protected]/node_modules/viem/_types/utils/formatters/transactionRequest.d.ts(21,1558): error TS2536: Type 'K_2' cannot be used to index type '{ [K_3 in keyof TransactionRequestEIP2930<`0x${string}`, `0x${string}`, "0x1"> as K_3 extends keyof TOverrideReturnType ? TOverrideReturnType[K_3] extends void ? never : K_3 : K_3]: K_3 extends keyof TOverrideReturnType ? TOverrideReturnType[K_3] : TransactionRequestEIP2930<...>[K_3]; } & TOverrideReturnType'.
../../node_modules/.pnpm/[email protected][email protected][email protected]/node_modules/viem/_types/utils/formatters/transactionRequest.d.ts(21,2522): error TS2536: Type 'K_4' cannot be used to index type '{ [K_5 in keyof TransactionRequestEIP1559<`0x${string}`, `0x${string}`, "0x2"> as K_5 extends keyof TOverrideReturnType ? TOverrideReturnType[K_5] extends void ? never : K_5 : K_5]: K_5 extends keyof TOverrideReturnType ? TOverrideReturnType[K_5] : TransactionRequestEIP1559<...>[K_5]; } & TOverrideReturnType'.

This is the same error mentioned in wevm/viem#1828 (comment), where it's suggested to enable the flag. It appears that viem and its examples have this flag turned on.

By setting "skipLibCheck": true, our tsc skips type checking of declaration files. I'll look into this to see if there's a better solution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"skipLibCheck": true is necessary for using viem, and even in the viem repository, the same error occurs when removing the "skipLibCheck": true. I believe there isn't a good way to maintain "skipLibCheck": false, although I don't fully understand the exact cause of the error.

Skipping the check should be fine here because the declaration files are not utilized in the packages where I enabled the flag, making it relevant only for library files.

@@ -17,7 +17,7 @@ export const accessManagementSystemDeployedBytecodeSize = size(
);
export const accessManagementSystemBytecode = encodeDeployData({
bytecode: accessManagementSystemBuild.bytecode.object as Hex,
abi: [],
abi: [] as Abi,
Copy link
Member

Choose a reason for hiding this comment

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

why do we have to cast this? I would expect TS to be able to infer this in the call to encodeDeployData

Copy link
Contributor Author

Choose a reason for hiding this comment

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

abi: [] is now not allowed for encodeDeployData. I think [] is not the abi expected here. I'll look into this.

src/deploy/ensureWorldFactory.ts:18:64 - error TS2345: Argument of type '{ bytecode: `0x${string}`; abi: never[]; }' is not assignable to parameter of type 'never'.

18 export const accessManagementSystemBytecode = encodeDeployData({
                                                                  ~
19   bytecode: accessManagementSystemBuild.bytecode.object as Hex,
   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
20   abi: [],
   ~~~~~~~~~~
21 });
   ~

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The encodeDeployData function does the task only if the contract has constructor args. It appears to have started rejecting abis with no effects at the type level? The logic of the function hasn't changed, so its behavior remains the same as before.

Alternatively, we can bypass this function:

diff --git a/packages/cli/src/deploy/ensureWorldFactory.ts b/packages/cli/src/deploy/ensureWorldFactory.ts
index 1f2913a8..f45cbed8 100644
--- a/packages/cli/src/deploy/ensureWorldFactory.ts
+++ b/packages/cli/src/deploy/ensureWorldFactory.ts
@@ -15,10 +15,7 @@ import { Contract } from "./ensureContract";
 export const accessManagementSystemDeployedBytecodeSize = size(
   accessManagementSystemBuild.deployedBytecode.object as Hex
 );
-export const accessManagementSystemBytecode = encodeDeployData({
-  bytecode: accessManagementSystemBuild.bytecode.object as Hex,
-  abi: [] as Abi,
-});
+export const accessManagementSystemBytecode = accessManagementSystemBuild.bytecode.object as Hex;
 export const accessManagementSystem = getCreate2Address({
   from: deployer,
   bytecode: accessManagementSystemBytecode,

Copy link
Member

@holic holic Feb 22, 2024

Choose a reason for hiding this comment

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

seems fine to leave the empty ABI approach! feels clearer to me

@@ -3,7 +3,6 @@ import type { MUDChain } from "./types";
export const latticeTestnet = {
name: "Lattice Testnet",
id: 4242,
network: "lattice-testnet",
Copy link
Member

Choose a reason for hiding this comment

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

for anyone reading: the network key got removed from Chain in latest viem (I think this was a ethers carry-over)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The network field was removed in this commit. It suggests using the id field instead when necessary.

@tash-2s tash-2s mentioned this pull request Feb 21, 2024
@tash-2s tash-2s changed the title upgrade viem to v2 build: upgrade viem to v2 Feb 22, 2024
@tash-2s tash-2s marked this pull request as ready for review February 22, 2024 04:02
Comment on lines -19 to +20
export function encodeSystemCall<abi extends Abi, functionName extends string = string>({
export function encodeSystemCall<abi extends Abi, functionName extends ContractFunctionName<abi>>({
abi,
systemId,
functionName,
args,
}: SystemCall<abi, functionName>): GetFunctionArgs<typeof IWorldCallAbi, "call">["args"] {
}: SystemCall<abi, functionName>): AbiParametersToPrimitiveTypes<
ExtractAbiFunction<typeof IWorldCallAbi, "call">["inputs"]
> {
Copy link
Contributor Author

@tash-2s tash-2s Feb 22, 2024

Choose a reason for hiding this comment

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

These encode call functions have been manually tested like this:

await worldContract.write.call(
  encodeSystemCall({
    abi: worldContract.abi,
    functionName: "addTask",
    args: [label],
    systemId: resourceToHex({ type: "system", namespace: "", name: "TasksSystem" }),
  })
);
await worldContract.write.batchCall([
  encodeSystemCalls(worldContract.abi, [
    {
      functionName: "addTask",
      args: [label],
      systemId: resourceToHex({ type: "system", namespace: "", name: "TasksSystem" }),
    },
  ]).map(([systemId, callData]) => ({ systemId, callData })),
]);
await worldContract.write.callFrom(
  encodeSystemCallFrom({
    from: walletClient.account.address,
    abi: worldContract.abi,
    functionName: "addTask",
    args: [label],
    systemId: resourceToHex({ type: "system", namespace: "", name: "TasksSystem" }),
  })
);
await worldContract.write.batchCallFrom([
  encodeSystemCallsFrom(worldContract.abi, walletClient.account.address, [
    {
      functionName: "addTask",
      args: [label],
      systemId: resourceToHex({ type: "system", namespace: "", name: "TasksSystem" }),
    },
  ]).map(([from, systemId, callData]) => ({ from, systemId, callData })),
]);

@holic holic changed the title build: upgrade viem to v2 feat: upgrade viem to v2 Feb 22, 2024
wallet: walletClient,
},
}) as unknown as GetContractReturnType<TAbi, { public: TPublicClient; wallet: TWalletClient }, TAddress> & {
write: unknown;
Copy link
Member

Choose a reason for hiding this comment

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

what does this bit do? do we need it?

Copy link
Contributor Author

@tash-2s tash-2s Feb 22, 2024

Choose a reason for hiding this comment

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

This corresponds to this viem code. At the type level, the properties attached to the contract object depend on the clients and the abi types. Since the actual abi type isn't known here, I needed to specify it to have the 'write' property. (To fix Property 'write' does not exist on type '... typescript error)
There might be a better way to handle this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While calling this MUD's getContract, it is typed the same as Viem's since it uses the same arguments and return type.

@@ -98,11 +108,11 @@ export function getContract<
args,
...options,
onWrite,
} as unknown as WriteContractParameters<TAbi, typeof functionName, TChain, TAccount>;
} as unknown as WriteContractParameters;
Copy link
Member

@holic holic Feb 22, 2024

Choose a reason for hiding this comment

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

are we losing type specificity/safety here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This loses some type specificity. I'll fix it.

return [
systemId,
encodeFunctionData({
abi,
functionName,
args,
} as unknown as EncodeFunctionDataParameters<abi, functionName>),
} as unknown as EncodeFunctionDataParameters),
Copy link
Member

Choose a reason for hiding this comment

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

same as above, are we losing any type specificity/safety here?

Copy link
Member

@holic holic left a comment

Choose a reason for hiding this comment

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

this is looking great!

just a few comments and I went ahead and added a changeset

@tash-2s
Copy link
Contributor Author

tash-2s commented Feb 22, 2024

@holic Thank you so much for the review and the changeset! I've addressed the comments and resolved the conflict.

@holic holic merged commit d7b1c58 into latticexyz:main Feb 23, 2024
10 of 11 checks passed
@tash-2s tash-2s deleted the viem-v2 branch February 23, 2024 13:48
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.

upgrade viem to v2
2 participants