Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
feat: create svm spoke _update #976
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
base: epic/svm-client
Are you sure you want to change the base?
feat: create svm spoke _update #976
Changes from 3 commits
0858448
6d2ab71
178c720
dc78474
5289074
34c13c0
9d97ebb
ef7bf92
4aa0073
392bf63
0d79629
bf5dda6
8eb864f
dac8c75
0cbfd9d
ff3aa9d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
One observation: here we're querying the latest block and then down in
eventsClient
that is translated to a slot number via binary search. I think we could short-circuit all this by queryingprovider.getSlot({ commitment: "confirmed" })
. Do you think theeventsClient
could accept something akin to an EVMBlockTag
, such thatconfirmed
results in a call toprovider.getSlot()
to resolve at the specified commitment, and a block number would result in a call togetSlotForBlock()
?(nb. we could also generalise
EventSearchConfig
by renamingfromBlock
->from
andtoBlock -> to
)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.
Fair - my only worry about something like this is that we're now moving away from the "blocks" model upstream
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.
RE slots and blocks, there's still nuance here because
toBlock
is a block, butgetBlockTime
expects a slot.I'm wondering if the SpokePoolClient needs to have responsibility for mapping blocks to slots internally. I'm hacking on an example of how we might do this.
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.
Wow that's super strange - the function takes a name called
blockNumber
but the full definition is below. Very good catchThere 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.
@james-a-morris Can we also now drop the
as DepositWithBlock
type assertion down on line 503 below?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.
Sure