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.

Sign up for GitHub

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

refactor(common): cleanup some inferred types, add TS benchmark #2956

Merged
merged 5 commits into from
Jul 19, 2024
Merged

refactor(common): cleanup some inferred types, add TS benchmark #2956

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
cfcefc9
perf(common): cleanup some inferred types, add vanilla template bench…
ssalbdivad Jul 18, 2024
e05e478
Merge branch 'main' into main
ssalbdivad Jul 18, 2024
886252e
add ts-benchmarks test package
holic Jul 19, 2024
49f7375
try dts
holic Jul 19, 2024
7cc25c0
clean up
holic Jul 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,3 +22,4 @@ test-data/world-logs-bulk-*.json
test-data/world-logs-query.json

.attest
.tstrace
2 changes: 2 additions & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
"package.json": "pnpm sort-package-json"
},
"devDependencies": {
"@ark/attest": "0.9.2",
Copy link
Member

Choose a reason for hiding this comment

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

I saw you did this in another PR and I had moved it out of the workspace root in a follow up PR to be consistent with how we've historically managed deps. Open to changing this approach though and curious to hear your thoughts - should common test deps be added to the workspace root instead of individual packages?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, devDependencies should generally be managed at the monorepo root in my opinion. It prevents having to manage multiple versions and continue to install them in new packages as needed, and there's really no downside as long as you're not importing them in source. I have an eslint rule that warns if I'm importing from a package not listed in dependencies.

Copy link
Member

Choose a reason for hiding this comment

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

cool, added a follow up here: #2962

"@changesets/cli": "^2.26.1",
"@types/node": "^18.15.11",
"@typescript-eslint/eslint-plugin": "7.1.1",
Expand All @@ -51,6 +52,7 @@
"prettier": "3.2.5",
"prettier-plugin-solidity": "1.3.1",
"rimraf": "^3.0.2",
"tsx": "4.16.2",
"turbo": "^1.9.3",
"typescript": "5.4.2"
},
Expand Down
28 changes: 14 additions & 14 deletions packages/common/src/getContract.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,16 +79,16 @@ export function getContract<
TPublicClient,
TWalletClient
>): GetContractReturnType<TAbi, { public: TPublicClient; wallet: TWalletClient }, TAddress> {
const contract = viem_getContract({
const contract: GetContractReturnType<TAbi, { public: TPublicClient; wallet: TWalletClient }, TAddress> & {
write: unknown;
} = viem_getContract({
abi,
address,
client: {
public: publicClient,
wallet: walletClient,
},
}) as unknown as GetContractReturnType<TAbi, { public: TPublicClient; wallet: TWalletClient }, TAddress> & {
write: unknown;
};
}) as never;
Copy link
Member

@holic holic Jul 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

is there any practical benefit to this approach?

iirc we copied the as unknown as ... pattern from viem, so I wonder if there's some improvements to be done there too

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, a few things:

  • Avoid duplicating long, complex expressions with no additional type safety
  • Avoid comparing potentially complex expressions that don't need to be compared because you're casting them anyways. This comparison shouldn't occur in the as unknown as X case but it would occur if e.g. you have an explicit return type and also case the return value in some areas to another complex expression instead of just never when the only goal is just to allow that return anyways.

Copy link
Member

Choose a reason for hiding this comment

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

happy with this change, but just FYI, I reverted this back to what's in main and I see no change in instantiations

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah I should have been clearer, I made this change while optimizing perf based on the traces I saw but it just led me to the variance issue in viem. I know there is no perfect impact here, and specifically casting as unknown as X for a function with an explicit return type of X will not cost anything.

The problem is if X is a long expression, not only is it unnecessary duplication, if the two ever get slightly out of sync, e.g. an extra optional parameter is added to the return but not the cast, then TS does have to compare the instantiations which can be very expensive.


if (contract.write) {
// Replace write calls with our own. Implemented ~the same as viem, but adds better handling of nonces (via queue + retries).
Expand All @@ -104,21 +104,21 @@ export function getContract<
]
) => {
const { args, options } = getFunctionParameters(parameters);
const request = {
const request: WriteContractParameters<
TAbi,
ContractFunctionName<TAbi, "nonpayable" | "payable">,
ContractFunctionArgs<TAbi, "nonpayable" | "payable">,
TChain,
TAccount
> = {
abi,
address,
functionName,
args,
...options,
onWrite,
} as unknown as WriteContractParameters<
TAbi,
ContractFunctionName<TAbi, "nonpayable" | "payable">,
ContractFunctionArgs<TAbi, "nonpayable" | "payable">,
TChain,
TAccount
>;
const result = writeContract(walletClient, request, { publicClient });
} as never;
const result = writeContract(walletClient, request, { publicClient }) as never;

const id = `${walletClient.chain.id}:${walletClient.account.address}:${nextWriteId++}`;
onWrite?.({
Expand All @@ -134,5 +134,5 @@ export function getContract<
);
}

return contract as unknown as GetContractReturnType<TAbi, { public: TPublicClient; wallet: TWalletClient }, TAddress>;
return contract;
}
Loading
Loading