-
Notifications
You must be signed in to change notification settings - Fork 992
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
Gas&fee updates #2091
Gas&fee updates #2091
Conversation
be607d4
to
e1386b4
Compare
e1386b4
to
aec3302
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.
Only some minor comments.
apps/src/bin/namada-node/cli.rs
Outdated
let _validator_local_config: ValidatorLocalConfig = | ||
toml::from_slice(&updated_config).unwrap(); | ||
|
||
// Update validator configuration file with the new accepted |
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.
presumably other updates would also be applied, if present in the file read?
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.
Yes I probably made it more generic than needed, at the moment the only local configuration parameter that we need is the gas&fee related one, but maybe we could add more in the future? This file though doesn't bind with the other configurations that we have (for example, we don't merge the toml files) so if we want it to be meaningful we need to also update the code to make use of these local parameters, otherwise we just read the file and do nothing (in a sense, it should be "safe" by default).
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 is fine, but we should make sure the comments and cli usage info match what the code actually does
@@ -111,6 +111,10 @@ pub const TX_WITHDRAW_WASM: &str = "tx_withdraw.wasm"; | |||
pub const TX_INIT_ACCOUNT_WASM: &str = "tx_init_account.wasm"; | |||
pub const TX_INIT_VALIDATOR_WASM: &str = "tx_init_validator.wasm"; | |||
|
|||
pub const TX_RESIGN_STEWARD: &str = "tx_resign_steward.wasm"; |
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.
how do these changes relate to this PR
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.
Ah, we were just missing the benchmark for this specific transaction, so I added it in this PR, nothing more than this
3b7df73
to
bfd8963
Compare
bfd8963
to
bbe3f98
Compare
bbe3f98
to
07fe6c9
Compare
Describe your changes
Addresses #1924.
Closes #1923.
Closes #2054.
Closes #2056.
This PR addressed the following:
Indicate on which release or other PRs this topic is based on
v0.25.0
Checklist before merging to
draft