-
-
Notifications
You must be signed in to change notification settings - Fork 821
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: multicall + contract error cleanup #56
Conversation
🦋 Changeset detectedLatest commit: 74b16a5 The changes in this PR will be included in the next version bump. 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 |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@@ -1,6 +1,16 @@ | |||
{ | |||
"files": { | |||
"ignore": ["cache", "coverage", "node_modules", "dist", "generated", "CHANGELOG.md", "pnpm-lock.yaml", ".next"] | |||
"ignore": [ | |||
"**/types/multicall.ts", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rome doesn't support some TypeScript syntax for the multicall types, so I'm excluding it for now.
Size Change: +1.15 kB (+3%) Total Size: 35.5 kB
ℹ️ View Unchanged
|
? (ContractConfig & TProperties)[] | ||
: TContracts extends [] | ||
? [] | ||
: TContracts extends [infer Head extends Contract] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rome doesn't like the [infer Head extends Contract]
part. 😢
name: ExtractNameFromAbi<TAbi, TFunctionName> | ||
} & Partial<ExtractArgsFromAbi<TAbi, TFunctionName>> | ||
|
||
export function getAbiItem< |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change to support function overloads. Similar to normalizeFunctionName
in wagmi.
decodedData = decodeErrorResult({ abi, data }) | ||
const { errorName, args: errorArgs } = decodedData | ||
if (errorName === 'Error') reason = (errorArgs as string[])[0] | ||
// TODO: Support Panic(uint256) & custom errors. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do this one in next PR.
Codecov upload limit reached
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good!
initialBlockNumber, | ||
publicClient, | ||
usdcContractConfig, | ||
vitalikAddress, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really part of the PR, but thoughts on making an address
lookup? Could make it easier to import and use in tests since you only need to know single import versus multiple.
const address = {
vitalik: '0x…',
'wagmi-dev.eth': '0x…',
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good idea! will do this in next pr
src/constants/abis.ts
Outdated
@@ -0,0 +1,556 @@ | |||
/* [Multicall3](https://github.com/mds1/multicall) */ | |||
export const multicallAbi = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we call this multicall3Abi
to be future-proof? I guess it might not matter though if it's just an implementation detail (e.g. we change to Multicall4 without needing to continue supporting Multicall3).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup
details: string | ||
docsPath?: string | ||
metaMessages?: string[] | ||
shortMessage: string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would I use shortMessage
to display to users in interface, etc.?
/** Contract address */ | ||
address: Address | ||
/** Function to invoke on the contract */ | ||
functionName: ExtractFunctionNameFromAbi< |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Random thought: Instead of functionName
should we call it just name
everywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same goes for eventName
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, probably makes sense! can do that in next PR!
multicall
humanMessage
toshortMessage