Skip to content

Conversation

dijanin-brat
Copy link
Contributor

Use SpokePoolManager instead of SpokePoolClients object to enforce devs to handle wrong/incorrect chainIds.

@dijanin-brat dijanin-brat self-assigned this Jul 2, 2025
@dijanin-brat dijanin-brat marked this pull request as ready for review July 2, 2025 15:24
Copy link
Member

@nicholaspai nicholaspai left a comment

Choose a reason for hiding this comment

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

How does this improve current code, I'm not quite following?

@dijanin-brat
Copy link
Contributor Author

How does this improve current code, I'm not quite following?

@nicholaspai Idea is to change every place where we are using spokePoolClients object directly with SpokePoolManager. Because, if we access the object directly like spokePoolClients[chainId], and there is no value for that specific key (chainId), it will return undefined but typescript will not enforce devs to handle that case. This is the first iteration of changes (it should be added on more places later) so we always handle cases where it can return undefined (so we can decide, in code, how to handle those cases and not have unexpected runtime errors). Does that make sense?


constructor(
protected readonly spokePoolClients: { [chainId: number]: SpokePoolClient },
spokePoolClients: { [chainId: number]: SpokePoolClient },
Copy link
Member

Choose a reason for hiding this comment

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

OOC why are we removing the protected + readonly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

spokePoolClients will not be class property anymore. It is replaced with SpokePoolManager and we will use spokePoolClients just as an argument for SpokePoolManager initialization.

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.

3 participants