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

fees: add unit tests #4316

Closed
wants to merge 1 commit into from
Closed

fees: add unit tests #4316

wants to merge 1 commit into from

Conversation

TalDerei
Copy link
Collaborator

@TalDerei TalDerei commented May 4, 2024

Describe your changes

I modified the unit tests for fees according to the updated planner logic.

Issue ticket number and link

Checklist before requesting a review

References #4155

  • If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason:

@TalDerei TalDerei marked this pull request as ready for review May 4, 2024 10:03
@TalDerei TalDerei self-assigned this May 4, 2024
@cratelyn cratelyn added A-testing Area: Relates to testing of Penumbra A-fees Area: Relates to transaction fees labels May 6, 2024
Copy link
Member

@erwanor erwanor left a comment

Choose a reason for hiding this comment

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

Thanks for adding tests, requesting changes because the planner api has changed since this PR was opened so this would break CI.

let fee = planner.fee_estimate(&planner.gas_prices, &planner.fee_tier);
planner.adjust_change_for_fee(fee);

if planner.balance().is_zero() {
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this cheating? The plan should balance once we run out of required notes if we have correctly refreshed the change notes

planner.actions = planner.plan.actions.clone();

let mut iterations = 0usize;
while let Some(_required) = planner
Copy link
Member

Choose a reason for hiding this comment

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

I think adding tests is a great idea, but my sense is that rather than replicating the logic (which has changed since this PR was opened) we should either:

Copy link
Member

Choose a reason for hiding this comment

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

My intuition is that it's probably best to wait a little longer until we try to test this, we'll definitely have less turbulence in the planner in a couple weeks time or less

@TalDerei
Copy link
Collaborator Author

TalDerei commented May 6, 2024

cc @cratelyn closing out since it's fallen out of sync with the latest round of refactors. This can be a reference for eventually integrating into the mock consensus suite.

@TalDerei TalDerei closed this May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-fees Area: Relates to transaction fees A-testing Area: Relates to testing of Penumbra
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants