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
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions .changeset/silly-geckos-agree.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"viem": minor
---

Improved return type for toSignature
139 changes: 139 additions & 0 deletions src/utils/hash/normalizeSignature.test-d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,139 @@
import { expectTypeOf, test } from 'vitest'
import { normalizeSignature } from './normalizeSignature.js'

/**
* Cases where we should be able to get proper types for this, but where the feature just isn't implemented in Viem
*/
type TodoType = string

test('foo()', () => {
expectTypeOf(normalizeSignature('event foo()')).toMatchTypeOf<'foo()'>()
})

test('bar(uint foo)', () => {
expectTypeOf(normalizeSignature('bar(uint foo)')).toMatchTypeOf<TodoType>()
})

test('processAccount(uint256 , address )', () => {
expectTypeOf(
normalizeSignature('processAccount(uint256 , address )'),
).toMatchTypeOf<TodoType>()
})

test('function bar()', () => {
expectTypeOf(normalizeSignature('function bar()')).toMatchTypeOf<'bar()'>()
})

test('function bar()', () => {
expectTypeOf(normalizeSignature('function bar( )')).toMatchTypeOf<TodoType>()
})

test('event Bar()', () => {
expectTypeOf(normalizeSignature('event Bar()')).toMatchTypeOf<'Bar()'>()
})

test('function bar(uint foo)', () => {
expectTypeOf(
normalizeSignature('function bar(uint foo)'),
).toMatchTypeOf<'bar(uint256)'>()
})

test('function bar(uint foo, address baz)', () => {
expectTypeOf(
normalizeSignature('function bar(uint foo, address baz)'),
).toMatchTypeOf<'bar(uint256,address)'>()
})

test('event Barry(uint foo)', () => {
expectTypeOf(
normalizeSignature('event Barry(uint foo)'),
).toMatchTypeOf<'Barry(uint256)'>()
})

test('Barry(uint indexed foo)', () => {
expectTypeOf(
normalizeSignature('Barry(uint indexed foo)'),
).toMatchTypeOf<TodoType>()
})

test('event Barry(uint indexed foo)', () => {
expectTypeOf(
normalizeSignature('event Barry(uint indexed foo)'),
).toMatchTypeOf<'Barry(uint256)'>()
})

test('function _compound(uint256 a, uint256 b, uint256 c)', () => {
expectTypeOf(
normalizeSignature('function _compound(uint256 a, uint256 b, uint256 c)'),
).toMatchTypeOf<'_compound(uint256,uint256,uint256)'>()
})

test('bar(uint foo, (uint baz))', () => {
expectTypeOf(
normalizeSignature('bar(uint foo, (uint baz))'),
).toMatchTypeOf<TodoType>()
})

test('bar(uint foo, (uint baz, bool x))', () => {
expectTypeOf(
normalizeSignature('bar(uint foo, (uint baz, bool x))'),
).toMatchTypeOf<TodoType>()
})

test('bar(uint foo, (uint baz, bool x) foo)', () => {
expectTypeOf(
normalizeSignature('bar(uint foo, (uint baz, bool x) foo)'),
).toMatchTypeOf<TodoType>()
})

test('bar(uint[] foo, (uint baz, bool x))', () => {
expectTypeOf(
normalizeSignature('bar(uint[] foo, (uint baz, bool x))'),
).toMatchTypeOf<TodoType>()
})

test('bar(uint[] foo, (uint baz, bool x)[])', () => {
expectTypeOf(
normalizeSignature('bar(uint[] foo, (uint baz, bool x)[])'),
).toMatchTypeOf<TodoType>()
})

test('foo(uint bar)', () => {
expectTypeOf(
normalizeSignature('foo(uint bar) payable returns (uint)'),
).toMatchTypeOf<TodoType>()
})

test('function submitBlocksWithCallbacks(bool isDataCompressed, bytes calldata data, ((uint16,(uint16,uint16,uint16,bytes)[])[], address[]) calldata config)', () => {
expectTypeOf(
normalizeSignature(
'function submitBlocksWithCallbacks(bool isDataCompressed, bytes calldata data, ((uint16,(uint16,uint16,uint16,bytes)[])[], address[]) calldata config)',
),
).toMatchTypeOf<'submitBlocksWithCallbacks(bool,bytes,((uint16,(uint16,uint16,uint16,bytes)[])[],address[]))'>(
'submitBlocksWithCallbacks(bool,bytes,((uint16,(uint16,uint16,uint16,bytes)[])[],address[]))',
)
})

test('function createEdition(string name, string symbol, uint64 editionSize, uint16 royaltyBPS, address fundsRecipient, address defaultAdmin, (uint104 publicSalePrice, uint32 maxSalePurchasePerAddress, uint64 publicSaleStart, uint64 publicSaleEnd, uint64 presaleStart, uint64 presaleEnd, bytes32 presaleMerkleRoot) saleConfig, string description, string animationURI, string imageURI) returns (address)', () => {
expectTypeOf(
normalizeSignature(
'function createEdition(string name, string symbol, uint64 editionSize, uint16 royaltyBPS, address fundsRecipient, address defaultAdmin, (uint104 publicSalePrice, uint32 maxSalePurchasePerAddress, uint64 publicSaleStart, uint64 publicSaleEnd, uint64 presaleStart, uint64 presaleEnd, bytes32 presaleMerkleRoot) saleConfig, string description, string animationURI, string imageURI) returns (address)',
),
).toMatchTypeOf<'createEdition(string,string,uint64,uint16,address,address,(uint104,uint32,uint64,uint64,uint64,uint64,bytes32),string,string,string)'>()
})

test('trim spaces', () => {
expectTypeOf(
normalizeSignature(
'function createEdition(string name,string symbol, uint64 editionSize , uint16 royaltyBPS, address fundsRecipient, address defaultAdmin, ( uint104 publicSalePrice , uint32 maxSalePurchasePerAddress, uint64 publicSaleStart, uint64 publicSaleEnd, uint64 presaleStart, uint64 presaleEnd, bytes32 presaleMerkleRoot) saleConfig , string description, string animationURI, string imageURI ) returns (address)',
),
).toMatchTypeOf<TodoType>()
})

test('error: invalid signatures', () => {
// TODO: these should be errors, but they aren't yet
// assertType<never>(normalizeSignature('bar'))
// assertType<never>(normalizeSignature('bar(uint foo'))
// assertType<never>(normalizeSignature('baruint foo)'))
// assertType<never>(normalizeSignature('bar(uint foo, (uint baz)'))
})
43 changes: 37 additions & 6 deletions src/utils/hash/normalizeSignature.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,44 @@
import type {
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

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

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

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 string inputs work. 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

* TODO: ParseAbiItem<Signature> is too strict here since
* 1. normalizeSignature allows loose formatting (ex: spacing)
* 2. normalization doesn't require the string to conform to abitype's human-readable encoding
* ex: 'function bar( )' should still results in `bar()` as the result
* ex: `event foo(tuple(uint16, uint16))` results in a broken type response
* for now, we return`string` in cases where abitype fails
* but this doesn't handle error cases where abitype is just wrong like
* TODO: Some obvious errors get the return type `string`
* this is because we fallback to `string` if `ParseAbiItem<Signature>` fails
* but it could fail because not because it's too strict, but because the string is plain incorrect
* ex: normalizeSignature('bar') falls back to `string` as the response type
* since it can't tell if this is wrong since `ParseAbiItem<Signature>` is too strict or if it really is invalid
* TODO: Some normalizations are missing
* ex 1: (JS wrong, type correct) 'event Barry(uint foo)' results in type `Barry(uint256)`, but the real return value is `Barry(uint)`.
* ex 2: (Both wrong) tuples doesn't get removed `foo(tuple(uint16, uint16))` does not get normalized to 'foo((uint16, uint16))'
*/
export type ToSignature<Signature extends string> =
ParseAbiItem<Signature> extends never
? string
: ParseAbiItem<Signature> extends AbiFunction | AbiEvent
? SignatureAbiItem<ParseAbiItem<Signature>>
: never

type NormalizeSignatureParameters<Signature extends string> = Signature
type NormalizeSignatureReturnType<Signature extends string> =
ToSignature<Signature>
export type NormalizeSignatureErrorType = ErrorType

export function normalizeSignature(
signature: NormalizeSignatureParameters,
): NormalizeSignatureReturnType {
export function normalizeSignature<Signature extends string>(
signature: NormalizeSignatureParameters<Signature>,
): NormalizeSignatureReturnType<Signature> {
let active = true
let current = ''
let level = 0
Expand Down Expand Up @@ -60,5 +91,5 @@ export function normalizeSignature(

if (!valid) throw new BaseError('Unable to normalize signature.')

return result
return result as NormalizeSignatureReturnType<Signature>
}
18 changes: 15 additions & 3 deletions src/utils/hash/toSignature.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,14 @@
import { type AbiEvent, type AbiFunction, formatAbiItem } from 'abitype'
import {
type AbiEvent,
type AbiFunction,
type SignatureAbiItem,
formatAbiItem,
} from 'abitype'

import type { ErrorType } from '../../errors/utils.js'
import {
type NormalizeSignatureErrorType,
type ToSignature,
normalizeSignature,
} from './normalizeSignature.js'

Expand All @@ -25,10 +31,16 @@ export type ToSignatureErrorType = NormalizeSignatureErrorType | ErrorType
* })
* // 'ownerOf(uint256)'
*/
export const toSignature = (def: string | AbiFunction | AbiEvent) => {
export const toSignature = <const Def extends string | AbiFunction | AbiEvent>(
def: Def,
): Def extends AbiFunction | AbiEvent
? SignatureAbiItem<Def>
: Def extends string
? ToSignature<Def>
: never => {
const def_ = (() => {
if (typeof def === 'string') return def
return formatAbiItem(def)
})()
return normalizeSignature(def_)
return normalizeSignature(def_) as ToSignature<typeof def_>
}