-
Notifications
You must be signed in to change notification settings - Fork 351
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(cli): Price Estimates in Aligned CLI #1577
base: staging
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Working nicely!
f6d7d9f
to
4e23678
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than the minor comments, everything is working correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't finished reviewing yet. Left some comments for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR was quite difficult to follow. I think it is more about the chosen functions/variables names rather than the actual logic.
Please take more time in defining good functions and variable names. This is more important than what most people think. Code should be written as if an external person with no context wants to read it. You need to be more precise on what a function does and what a variable represents.
Better code clarity = less bugs & better maintainability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code looks way better, wip testing
Price Estimates in Aligned CLI
Description
Addresses No. 1 in #1570. Adds a new pricing parameter for the cli using
PriceEstimate
from the sdk. Refactors some parts of the sdk.Type of change
Please delete options that are not relevant.
To Test:
Not specifying a fee should use the default.
--max_fee
--default_fee_estimate
--instant_fee_estimate
`--custom_fee_estimate <NUM_PROOFS_IN_BATCH>"
max_fee
anddefault_fee_estimate/instant_fee_estimate/custom_fee_estimate
should not be displayed.Clap errors if custom does not have <NUM_PROOFS_PER_BATCH>
Parsing errors for "--custom_fee_estimate" if incorrect value is supplied for
num_proofs_in_batch
Checklist
testnet
, everything else tostaging