Skip to content
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

Implement versioned logic module routing for fee-quoter #604

Merged
merged 3 commits into from
Feb 13, 2025

Conversation

toblich
Copy link
Contributor

@toblich toblich commented Feb 12, 2025

We created versioned modules with upgradeable business logic in #414 and subsequent PRs. This new PR implements for FeeQuoter the routing layer between versions of said module.

Note for reviewers: make sure to hide whitespaces in GitHub when reviewing this to reduce some of the noise.

@toblich toblich marked this pull request as ready for review February 12, 2025 21:19
@toblich toblich requested a review from a team as a code owner February 12, 2025 21:19
Copy link
Contributor

@PabloMansanet PabloMansanet left a comment

Choose a reason for hiding this comment

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

LGTM!

Comment on lines +11 to +20
pub trait Public {
fn get_fee<'info>(
&self,
ctx: Context<'_, '_, 'info, 'info, GetFee>,
dest_chain_selector: u64,
message: SVM2AnyMessage,
) -> Result<GetFeeResult>;
}

pub trait Prices {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure about the trait names; I think they could be a bit more expressive. PublicFeeGetter, UpdateablePrices maybe? As it stands it can be difficult to tell what functionality is supported by just reading the trait.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They correlate 1:1 with module names. I'm up for renaming them if you prefer, but then I would also rename files & modules everywhere to keep them aligned. I'll merge as-is and do any renaming in a separate PR after we've chatted more offline 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, if the semantics are "the trait refers to who can call this", Public and Admin make sense, though I'd still like a clarifying comment atop the trait itself. However, Prices stands out among the three then. Maybe it just needs to be PriceUpdater so it also responds to the question of who can call this.

@toblich toblich enabled auto-merge (squash) February 13, 2025 14:42
Copy link

Metric tobi/fee-quoter-logic-version-router main
Coverage 74.5% 74.0%

@toblich toblich merged commit fd1731e into main Feb 13, 2025
16 of 17 checks passed
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