-
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
Move some constants & types out of fvm_shared #1517
Conversation
More to come, but this covers some of the easy stuff. |
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.
LGTM whether or not you delete SectorQuality.
actors/market/src/policy.rs
Outdated
use num_traits::Zero; | ||
|
||
pub mod detail { | ||
/// Maximum length of a deal label. | ||
pub const DEAL_MAX_LABEL_SIZE: usize = 256; | ||
} | ||
|
||
lazy_static! { | ||
/// Total Filecoin available to the network. |
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.
Commentary: The assumed total filecoin. We should really make this a syscall.
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.
TBH, this constant should just go away. It's used to specify collateral bounds, which is kind of unnecessary IMO (although I guess the idea is to limit the size of state?).
use fvm_shared::clock::EPOCH_DURATION_SECONDS; | ||
pub use fvm_shared::BLOCKS_PER_EPOCH as EXPECTED_LEADERS_PER_EPOCH; | ||
|
||
pub const EPOCH_DURATION_SECONDS: i64 = 30; |
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.
Commentary: again assumed duration, but isn't something that gets defined by the actors.
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, but changing this would require a massive migration so I think it's OK if the actors assume a specific duration. But I'll comment on that.
07815ae
to
e0f95e1
Compare
e0f95e1
to
7ca2b49
Compare
I've removed the type aliases and added a bunch of comments. |
Specifically:
MAX_SECTOR_NUMBER
Spacetime
SectorQuality
TOTAL_FILECOIN
EPOCH_DURATION_SECONDS
BLOCKS_PER_EPOCH