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

stake: Delegating an amount below the minimum returns a misleading error message #25337

Closed
brooksprumo opened this issue May 18, 2022 · 5 comments · Fixed by #25709
Closed

stake: Delegating an amount below the minimum returns a misleading error message #25337

brooksprumo opened this issue May 18, 2022 · 5 comments · Fixed by #25709
Assignees

Comments

@brooksprumo
Copy link
Contributor

brooksprumo commented May 18, 2022

Problem

When the following features are enabled, delegating a stake amount below the minimum will return with an error saying "Error: split amount is more than is staked", which is misleading since the error was caused during delegate and not split.

Features:

Code in question:

#[error("split amount is more than is staked")]
InsufficientStake,

Originally encountered in #25336

Proposed Solution

Reword the error message.
Add a new error behind a feature-gate.

@brooksprumo
Copy link
Contributor Author

cc: @joncinque

@joncinque
Copy link
Contributor

Bah, sorry about that, we need to actually add a new error and return that finally, based on another feature flag probably

@brooksprumo
Copy link
Contributor Author

Bummer. It is insufficient to just reword the error message?

@joncinque
Copy link
Contributor

I think it could cause issues, since during rollout some nodes would produce the old error message, and others produce the new one. It depends on how those error messages are used by the runtime, but I'm cautious due to this note:

/// Failed to serialize or deserialize account data
///
/// Warning: This error should never be emitted by the runtime.
///
/// This error includes strings from the underlying 3rd party Borsh crate
/// which can be dangerous because the error strings could change across
/// Borsh versions. Only programs can use this error because they are
/// consistent across Solana software versions.
///
#[error("Failed to serialize or deserialize account data: {0}")]
BorshIoError(String),

Maybe @jackcmay can tell us for sure what we should do since he put in #17864

@jackcmay
Copy link
Contributor

Yeah, changing the String value without a feature flag is a bad idea because the return value is part of the bank hash

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants
@brooksprumo @joncinque @jackcmay and others