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

fix(en): more correct fees algorithm for external-node #3487

Closed
wants to merge 7 commits into from

Conversation

tomg10
Copy link
Contributor

@tomg10 tomg10 commented Jan 16, 2025

No description provided.

@tomg10 tomg10 requested a review from perekopskiy January 16, 2025 16:01
Copy link
Contributor

@perekopskiy perekopskiy left a comment

Choose a reason for hiding this comment

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

Please add PR description

core/node/fee_model/src/l1_gas_price/main_node_fetcher.rs Outdated Show resolved Hide resolved
core/lib/types/src/fee_model.rs Outdated Show resolved Hide resolved
core/node/fee_model/src/l1_gas_price/main_node_fetcher.rs Outdated Show resolved Hide resolved
params.conversion_ratio(),
));
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need all of this logic? Can't we just override get_batch_fee_input_scaled instead and return *self.main_node_batch_fee_input.read().unwrap() there? EN's scaling factor will be ignored but arguably this logic is not designed well in general (i.e. is there a reason to scale gas price differently on EN?). Also, fn get_fee_model_params(&self) -> FeeParams being a part of the base trait is strange too, it's more like an implementation detail for half of the implementations.

github-merge-queue bot pushed a commit that referenced this pull request Jan 17, 2025
## What ❔

Alternative to #3487

Before this PR EN based its gas pricing solely on main node's fee params
which are based on immediate market conditions and not representative of
the open batch's fee input. This is a problem on environments with
long-running batches. This PR makes EN fetch main node's batch fee input
along with fee params and report fetched batch fee input from its fee
input provider.

## Why ❔

<!-- Why are these changes done? What goal do they contribute to? What
are the principles behind them? -->
<!-- Example: PR templates ensure PR reviewers, observers, and future
iterators are in context about the evolution of repos. -->

## Checklist

<!-- Check your PR fulfills the following items. -->
<!-- For draft PRs check the boxes as you complete them. -->

- [x] PR title corresponds to the body of PR (we generate changelog
entries from PRs).
- [ ] Tests for the changes have been added / updated.
- [x] Documentation comments have been added / updated.
- [x] Code has been formatted via `zkstack dev fmt` and `zkstack dev
lint`.
@tomg10 tomg10 closed this Jan 17, 2025
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