-
Notifications
You must be signed in to change notification settings - Fork 80
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
[WIP] Remove unused and deprecated code #1431
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.
Thanks, but I think you are currently lacking context to target good clean-ups here.
@@ -32,8 +32,6 @@ fil_actors_runtime::wasm_trampoline!(Actor); | |||
pub enum Method { | |||
Constructor = METHOD_CONSTRUCTOR, | |||
PubkeyAddress = 2, | |||
// Deprecated in v10 | |||
// AuthenticateMessage = 3, |
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.
I don't think we should remove these (in any actor). They are useful documentation of method numbers that have been used, and relevant to interpreting old blockchain history. These lines aren't costing us anything to retain.
pub replace_capacity: bool, | ||
/// Deprecated: | ||
/// The committed capacity sector to replace, and its deadline/partition location | ||
pub replace_sector_deadline: u64, |
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.
You can't change Params structs. This is a publicly exported API invoked from off-chain. This will break existing callers. We can one day deprecate the entire method, forcing every remaining caller to migrate to PreCommitSectorBatch2, but until then it needs to remain as-is. Deprecated doesn't mean we can delete it – it means we can't delete it but people should stop using it.
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.
@anorth For my ow understanding, what kind of offchain callers use this public API ?
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.
For this one, storage providers. I haven't looked at stats recently to see how many have migrated to v2, but probably most have not yet.
#[allow(dead_code)] | ||
pub faulty_power: PowerPair, | ||
#[allow(dead_code)] | ||
pub on_time_pledge: TokenAmount, |
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.
These being dead signals to me that we are missing state invariant checks upstream of this that should use the values. I don't think removing them now is helpful – we should strengthen the invariant checks to use them instead.
Closing as this needs more context and alignment. |
First pass at removing unused and deprecated code from built in Actors.
TODO