-
-
Notifications
You must be signed in to change notification settings - Fork 972
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: getLogs #44
feat: getLogs #44
Conversation
🦋 Changeset detectedLatest commit: 991066b 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 ↗︎
|
Size Change: +367 B (+1%) Total Size: 33.1 kB
ℹ️ View Unchanged
|
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.
Good start!
I agree that we should provide a way to pass in raw topics
.
However, maybe we should include support for that in a future PR, and keep the API consistent with createEventFilter
? Otherwise, we should add topics
support to createEventFilter
.
My suggestion for now would be:
await getLogs(publicClient, {
address: '0x...',
event: 'Transfer(...)',
args: [...]
})
Then in a future PR, we should add support to be more flexible (multiple events, raw topics). Here are a few options:
Option 1: Support a union of params
This option explores supporting a union of params: { event, args } | { events } | { topics }
Single event w/ args
await getLogs(publicClient, {
address: '0x...',
event: 'Transfer(...)',
args: [...]
})
Multiple events w/o args
await getLogs(publicClient, {
address: '0x...',
events: ['Transfer(...)', 'Foo(...)'],
})
Raw topics
await getLogs(publicClient, {
address: '0x...',
topics: ['0x...', '0x...', ['0x...', '0x...']]],
})
Think the downside to this approach is that DX might be a bit harder to navigate which parameters belong in the union without referencing the docs.
Option 2: Implement a "builder" function
Option 2 could be to suggest what you said in your PR, to have a builder helper.
Single event w/ args
await getLogs(publicClient, {
address: '0x...',
topics: buildEventTopics({
event: 'Transfer(...)',
args: [...]
})
})
Multiple events w/o args
await getLogs(publicClient, {
address: '0x...',
topics: buildEventsTopics({
events: ['Transfer(...)', 'Foo(...)']
})
})
Raw topics
await getLogs(publicClient, {
address: '0x...',
topics: ['0x...', '0x...', ['0x...', '0x...']]],
})
I think I might like this approach better as:
- it better guards the consumer from inserting the incorrect union parameters,
- less magic behind the scenes, so it allows the consumer to learn how Ethereum log filters are built (they are a set of topics).
site/docs/actions/public/getLogs.md
Outdated
import { getLogs } from 'viem' | ||
import { publicClient } from '.' | ||
|
||
const logs = await getLogs(publicClient, { address, topics, fromBlock, toBlock }) |
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.
We should populate these attributes with example values.
site/docs/actions/public/getLogs.md
Outdated
|
||
### blockHash | ||
|
||
- **Type:** [`bigint | 'latest' | 'earliest' | 'pending' | 'safe' | 'finalized'`](/docs/glossary/types#TODO) |
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.
- **Type:** [`bigint | 'latest' | 'earliest' | 'pending' | 'safe' | 'finalized'`](/docs/glossary/types#TODO) | |
- **Type:** `'0x${string}'` |
site/docs/actions/public/getLogs.md
Outdated
|
||
- **Type:** [`bigint | 'latest' | 'earliest' | 'pending' | 'safe' | 'finalized'`](/docs/glossary/types#TODO) | ||
|
||
Block number or tag to include logs from. Mutually exclusive with `fromBlock`/`toBlock`. |
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.
Block number or tag to include logs from. Mutually exclusive with `fromBlock`/`toBlock`. | |
Block hash to include logs from. Mutually exclusive with `fromBlock`/`toBlock`. |
src/types/log.ts
Outdated
export type GetLogsParameters<TQuantity = bigint> = { | ||
/** Address or list of addresses from which logs originated */ | ||
address?: Address | Address[] | ||
/** List of order-dependent topics */ | ||
topics?: Hex[] | ||
} & ( | ||
| { | ||
/** Block number or tag after which to include logs */ | ||
fromBlock?: BlockNumber<TQuantity> | BlockTag | ||
/** Block number or tag before which to include logs */ | ||
toBlock?: BlockNumber<TQuantity> | BlockTag | ||
blockHash?: never | ||
} | ||
| { | ||
fromBlock?: never | ||
toBlock?: never | ||
/** Hash of block to include logs from */ | ||
blockHash?: Hash | ||
} | ||
) | ||
|
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.
Let's put this in getLogs.ts
.
2ac832a
to
874769c
Compare
Adds a
getLogs
action. Some discussion:API
The API proposed here is basically the RPC API, which is at odds with the current API for
eth_newFilter
via thecreateEventFilter
action. Per @jxom suggestion, I think it probably makes sense to consolidate these. One idea would be to replace thetopics
parameter entirely.The
buildEventFilter
function could be typed to allow either:which could help eliminate confusion around how topic sets work (and cover >99% of the ways people use topic sets).
Another idea would be to leave the
getLogs
API alone, and add a functionbuildTopicSet
that works the same way as the proposedbuildEventFilter
but returns the topic set as a(Hex | Hex[])[]
.