Skip to content

[CAL][13] - implement NextSeqNum and GetSourceChainConfig #1049

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ogtownsend
Copy link
Contributor

@ogtownsend ogtownsend commented Jun 25, 2025

@ogtownsend ogtownsend force-pushed the ogt/cal-13-implement-NextSeqNum-and-add-GetSourceChainConfig branch 4 times, most recently from 20cfa4b to 759e4d8 Compare June 25, 2025 21:02
@ogtownsend ogtownsend marked this pull request as ready for review June 25, 2025 21:18
@ogtownsend ogtownsend requested a review from a team as a code owner June 25, 2025 21:18
}

// GetSourceChainConfigs always fetches fresh source chain configs directly from contracts
// without using any cached values. Use this when up-to-date data is critical, especially
Copy link
Contributor

@winder winder Jun 26, 2025

Choose a reason for hiding this comment

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

I'm a little on the fence about putting this in the interface.

On one hand it avoids any confusion about whether the data is fresh or cached, so it's much clearer than the code that you've replaced. On the other, it's sort of an implementation detail because a non-EVM chain family might choose to store the next seq number in a different location.

Copy link
Contributor Author

@ogtownsend ogtownsend Jun 26, 2025

Choose a reason for hiding this comment

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

hmm, but accessor.NextSeqNum() calling accessor.GetSourceChainConfigs() is more of an implementation detail of this DefaultAccessor rather than the entire ChainAccessor interface

By adding GetSourceChainConfigs() to the CAL interface we're just saying all chains will need to provide a way to fetch source chain configs, but all future accessors won't require that NextSeqNum() calls GetSourceChainConfigs()...if that makes sense

@ogtownsend ogtownsend force-pushed the ogt/cal-13-implement-NextSeqNum-and-add-GetSourceChainConfig branch 2 times, most recently from c571391 to fe7cb40 Compare June 26, 2025 23:07
@ogtownsend ogtownsend force-pushed the ogt/cal-13-implement-NextSeqNum-and-add-GetSourceChainConfig branch from fe7cb40 to 04e45eb Compare June 27, 2025 19:07
@ogtownsend ogtownsend force-pushed the ogt/cal-13-implement-NextSeqNum-and-add-GetSourceChainConfig branch from 04e45eb to 58fbb25 Compare June 27, 2025 20:28
Copy link

Metric ogt/cal-13-implement-NextSeqNum-and-add-GetSourceChainConfig main
Coverage 69.7% 69.7%

@ogtownsend ogtownsend marked this pull request as draft July 14, 2025 22:05
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.

2 participants