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(svmSpokeUtils): get fill deadline buffer implementation #978
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(svmSpokeUtils): get fill deadline buffer implementation #978
Changes from 3 commits
7cd664b
9cf2918
fe2b225
d7e3dd6
d37346b
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.
For consistency should programId also be
Address
?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.
d7e3dd6
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.
I've added
programId
here because I think it's useful to have direct access to it. That said, I'm also not sure this is the best approach sincesvmSpokeEventsClient
already resolvesprogramId
internally, and that value might differ from the one passed in here. To address that I think we can either (1) use the resolved value fromsvmEventsClient
or (2) let the callers provideprogramId
and pass that tosvmSpokeEventsClient
.For option 1, we only need to make
svmSpokeAddress
a public property (here).For option 2 we'll have to remove the resolution logic from
svmSpokeEventsClient
entirely and turn it into a constructor param instead.I'm curious to hear other's thoughts about this. Personally, I'm leaning toward option 2. I think it's closer to what we have for EVM and it also improves flexibility. As an example, I had to override the program signature to test some Solana features while working in the Indexer and there was other way to do so than changing the
svmSpokeEventsClient
.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.
@melisaguevara Is it necessary for the eventsClient to be instantiated outside of the SVMSpokePoolClient? Could we instead instantiate it entirely within the
.create()
static method?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.
Fwiw I think it makes sense to also expose the
svmSpokeAddress
- it's not required to be private, and could theoretically be encapsulated in a getter fnThere 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.
@pxrl I don't think so, I think we'll always use it through the clients. This could be a follow up if we all agree to do so.
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.
@james-a-morris Yes I saw you did that in your PR. I think that's also an option, though as an end user you would have to do
svmSpokeClient.svmEventsClient.getSvmSpokeAddress()
. I think it would be nice to have that directly as property of SvmSpokeClient. For now I'll follow your approach but I'm also including it as a spoke client property, we can revisit this later.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 is how it looks for now: d7e3dd6
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; I think it's probably cleaner to incorporate it directly into the SpokePoolClient, rather than requiring it to be supplied as an external dependency. This should make it easier to instantiate.
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.
I think we can modify this after this PR goes in: #982