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

feat(fee): add FromStr, Display impls for FeeTier #4539

Merged
merged 1 commit into from
Jun 4, 2024

Conversation

conorsch
Copy link
Contributor

@conorsch conorsch commented Jun 4, 2024

Describe your changes

We recently enabled fees on Testnet 77 (#4306), and in the process broke a few things, such as Galileo. While working on adding FeeTier support to Galileo, Galileo's clap setup wanted Display and FromStr. Added those to satisfy the build, and then circled back to pcli to reuse the same impls, which allowed us to snip out some duplicative code.

Issue ticket number and link

Checklist before requesting a review

  • 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:

    no changes to app logic, just adding more impls

We recently enabled fees on Testnet 77 (#4306), and in the process
broke a few things, such as Galileo. While working on adding FeeTier
support to Galileo, Galileo's clap setup wanted Display and FromStr.
Added those to satisfy the build, and then circled back to pcli to reuse
the same impls, which allowed us to snip out some duplicative code.
@conorsch conorsch requested a review from cratelyn June 4, 2024 16:41
conorsch added a commit to penumbra-zone/galileo that referenced this pull request Jun 4, 2024
During Testnet 77 we enabled fees [0] and in the process broke some
things like Galileo. Adding FeeTier support is rather straightforward,
although this changeset requires a patch to the `penumbra-fee` crate in
order to compile [1].

[0] penumbra-zone/penumbra#4306
[1] penumbra-zone/penumbra#4539
@cratelyn cratelyn added the A-fees Area: Relates to transaction fees label Jun 4, 2024
@cratelyn cratelyn added this to the Sprint 8 milestone Jun 4, 2024
Copy link
Contributor

@cratelyn cratelyn left a comment

Choose a reason for hiding this comment

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

nice! thanks for additionally cleaning up the duplicate FeeTier in pcli while you did this. i appreciate the thought, and attention to detail. great work @conorsch!

@zbuc
Copy link
Member

zbuc commented Jun 4, 2024

Does this still select "low" by default if no tier is chosen?

@cratelyn
Copy link
Contributor

cratelyn commented Jun 4, 2024

Does this still select "low" by default if no tier is chosen?

https://github.com/penumbra-zone/penumbra/pull/4539/files#diff-87de50c1ecb12dbe4371567b45a2b49fa5b793bc4bcee7cab6776cc40ea5d61dR145-R149

impl Default for FeeTier {
    fn default() -> Self {
        Self::Low
    }
}

yes! see here ☝️

@conorsch conorsch merged commit 8035e29 into main Jun 4, 2024
13 checks passed
@conorsch conorsch deleted the 4306-fee-tier-string-impls branch June 4, 2024 17:25
conorsch added a commit to penumbra-zone/galileo that referenced this pull request Jun 4, 2024
During Testnet 77 we enabled fees [0] and in the process broke some
things like Galileo. Adding FeeTier support is rather straightforward,
although this changeset requires a patch to the `penumbra-fee` crate in
order to compile [1].

[0] penumbra-zone/penumbra#4306
[1] penumbra-zone/penumbra#4539
conorsch added a commit to penumbra-zone/galileo that referenced this pull request Jun 4, 2024
During Testnet 77 we enabled fees [0] and in the process broke some
things like Galileo. Adding FeeTier support is rather straightforward,
although this changeset requires a patch to the `penumbra-fee` crate in
order to compile [1].

[0] penumbra-zone/penumbra#4306
[1] penumbra-zone/penumbra#4539
cratelyn pushed a commit to penumbra-zone/galileo that referenced this pull request Jun 4, 2024
During Testnet 77 we enabled fees [0] and in the process broke some
things like Galileo. Adding FeeTier support is rather straightforward,
although this changeset requires a patch to the `penumbra-fee` crate in
order to compile [1].

[0] penumbra-zone/penumbra#4306
[1] penumbra-zone/penumbra#4539
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
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants