-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
remove fee_calculator from nonce data #34959
remove fee_calculator from nonce data #34959
Conversation
c80c38c
to
264b7b2
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.
@@ -87,18 +87,16 @@ pub struct TransactionExecutionDetails { | |||
pub accounts_data_len_delta: i64, | |||
} | |||
|
|||
// TODO - remove `DurableNonceFee` as it'd always be Invalid. |
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.
Should have a prequel PR to remove DurableNonceFee
first? Will that break something else?
nonce_account.lamports_per_signature = Some(data.fee_calculator.lamports_per_signature); | ||
// TODO - shoudl also nonce_account's lamports_per_signature in this PR? or do it in | ||
// separate one. | ||
nonce_account.lamports_per_signature = None; |
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.
Can remove optional lamports_per_signature
from CliNonceAccount
in separate PR, but should we keep it for now as deprecated if that's break cli client?
Ok((data.blockhash(), data.fee_calculator)) | ||
Self::NonceAccount(ref _pubkey) => { | ||
// Note - to break or to gut entire deprecated function? | ||
Err("Deprecated, NonceAccount no longer support fee_calculator".into()) | ||
} |
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.
Is it time to remove deprecated get_blockhash_and_fee_calculator()
function, (and one more below)? Or changes here to return Err is acceptable?
@@ -21,21 +20,17 @@ pub struct Data { | |||
pub authority: Pubkey, | |||
/// Durable nonce value derived from a valid previous blockhash. | |||
pub durable_nonce: DurableNonce, | |||
/// The fee calculator associated with the blockhash. | |||
pub fee_calculator: FeeCalculator, |
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 the main change, everything else are the consequences
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.
removing the field breaks consensus. all of the pub
symbol removals in sdk break semver.
if we want to do this, it has to be done under a new version variant and support for the older version(s) lives until all older accounts have been upgraded (read: forever). i think i'd just leave the bytes. we can mark all of the accessors deprecated so they can be removed in 2.0
I'd be in favor of a change that stores the |
Problem
Nonce has FeeCalculator that is not used anymore.
Summary of Changes
fee_calculator
from Nonce accountRequires #34943
Fixes #34864