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: Improve return type for toSignature #2928

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

SebastienGllmt
Copy link

@SebastienGllmt SebastienGllmt commented Oct 25, 2024

Integrates wevm/abitype#255 into Viem to improve the return type of toSignature

This helps resolve these two discussions: #260 and #2706

A follow-up PR to this will have to be adding the option to pass the full signature to functions like getContractEvents and getAbiItem


PR-Codex overview

This PR focuses on improving the type safety and return types of the toSignature and normalizeSignature functions in the codebase, enhancing the handling of ABI signatures.

Detailed summary

  • Updated toSignature to use generics for better return types.
  • Enhanced normalizeSignature with stricter type definitions.
  • Added detailed comments addressing limitations and expected behavior of signature normalization.
  • Introduced new tests to validate the changes in type handling.

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

Copy link

changeset-bot bot commented Oct 25, 2024

🦋 Changeset detected

Latest commit: ca1ca16

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

This PR includes changesets to release 1 package
Name Type
viem Minor

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

Copy link

vercel bot commented Oct 25, 2024

@SebastienGllmt is attempting to deploy a commit to the Wevm Team on Vercel.

A member of the Team first needs to authorize it.

import { BaseError } from '../../errors/base.js'
import type { ErrorType } from '../../errors/utils.js'

type NormalizeSignatureParameters = string
type NormalizeSignatureReturnType = string
/**
Copy link
Author

@SebastienGllmt SebastienGllmt Oct 25, 2024

Choose a reason for hiding this comment

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

There are multiple TODOs here and I'm not sure which one of these would have to be fixed to be considered good enough to merge (given that any type is better than no type, but the wrong type is also worse than no type)

I wanted to make the original commit for this PR contain as detailed information before we start truncating out cases to support the subset that makes sense

Here are the 4 options in increase level of work required (each option requires the previous option to be completed):

Option 1: we can make that the toSignature function only returns a type for AbiFunction | AbiEvent (since those cases are always correct) and then just return string otherwise (instead of trying to parse the string)

Option 2: we make string inputs work in some cases, but not all. This would require:
a. fixing the normalization of uint to uint256 (this is a bug in normalizeSignature and not in the type)
b. arguably, fixing the tuple(...) notation handling (but if the JS code is also wrong, no point in making the type match it)

Option 3: we try to make all valid string inputs work, but no guarantee on invalid inputs. This would require:
a. fixing tuple(uint) handling (maybe this is an abitype concern? However, this syntax isn't part of their human-readable string encoding)
b. adding proper trimming logic on whitespace

Option 4: make all string inputs work, and properly return never on invalid input

AbiEvent,
AbiFunction,
ParseAbiItem,
SignatureAbiItem,
Copy link
Author

@SebastienGllmt SebastienGllmt Oct 25, 2024

Choose a reason for hiding this comment

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

note: this type (SignatureAbiItem) depends on wevm/abitype#255 getting merged and released

@jxom jxom force-pushed the main branch 10 times, most recently from 5c14479 to 373eafa Compare November 20, 2024 23:37
@SebastienGllmt
Copy link
Author

SebastienGllmt commented Nov 24, 2024

Anything at all I can do to help move this forward?

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.

1 participant