-
Notifications
You must be signed in to change notification settings - Fork 17
feat!: simplify dataset status to two-state lifecycle (Inactive/Active) #314
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
Conversation
| @@ -0,0 +1,942 @@ | |||
| # Dataset Status Integration Guide | |||
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'm not reviewing this or the other .md file unless someone explicitly tells me to. It is probably best to either remove it or have someone rewrite it into a few paragraphs.
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.
Removed the added .md-files in the PR here: 6408bdc. Will open a draft-PR with those, so that we can get proper review and edit of those
| // Event for dataset status changes | ||
| event DataSetStatusChanged( | ||
| uint256 indexed dataSetId, DataSetStatus indexed oldStatus, DataSetStatus indexed newStatus, uint256 epoch | ||
| ); |
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.
Why are we adding an event? It is the first time I'm hearing about it.
Also, newStatus almost for sure should not be indexed, it just isn't worth indexing.
| // Dataset is inactive: non-existent (pdpRailId==0) or no pieces added yet (rate==0, no proving) | ||
| Inactive, | ||
| // Dataset has pieces and proving history (includes terminated datasets) | ||
| // Note: Terminated datasets remain Active - use pdpEndEpoch to check termination status |
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 comment is wrong. The datasets in the process of being terminated remain Active, but Terminated datasets will become Inactive as their data will be wiped when provider deletes the dataset from PDP side.
| */ | ||
| function isDataSetActive(FilecoinWarmStorageService service, uint256 dataSetId) internal view returns (bool) { | ||
| return getDataSetStatus(service, dataSetId) == FilecoinWarmStorageService.DataSetStatus.Active; | ||
| } |
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 getter serves no purpose
| hasProving = activationEpoch != 0; | ||
|
|
||
| // Check if terminated | ||
| isTerminated = info.pdpEndEpoch != 0; |
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 terminating not terminated
| * @return hasProving Whether proving is activated | ||
| * @return isTerminated Whether the rail is terminated | ||
| */ | ||
| function getDataSetStatusDetails(FilecoinWarmStorageService service, uint256 dataSetId) |
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 method seems counter to the whole PR. We decide to reduce the fidelity of status, but now this exposes even more internal detail.
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.
@rjan90 are you sure we want to keep this?
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.
Removed in the latest commit: 6408bdc
|
|
||
| // Inactive only if no proving has started | ||
| // Everything else is Active (including terminated datasets) | ||
| if (!hasProving) { |
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.
combine this with the line above, as the layer of indirection just makes it more confusing
|
If we remove |
Implement simplified dataset status system as specified in issue #169. Replaces the 3-state system (NotFound/Active/Terminating) with a clearer 2-state system (Inactive/Active). Key changes: - Updated DataSetStatus enum from 3 states to 2 states (Inactive/Active) - Status now reflects data existence, not operational state - Inactive = Dataset doesn't exist or has no pieces - Active = Dataset has data and proving history (including terminated) - Updated contract logic, tests, subgraph, and documentation BREAKING CHANGE: DataSetStatus enum values changed. Terminated datasets are now Active (not Terminating). Check operational state using pdpEndEpoch or isTerminated() instead.
522ae83 to
ddd6ccd
Compare
fix: update ABIs
Implements simplified dataset status system as specified in issue #169, replacing the 3-state system with a clearer 2-state system.
Summary
This PR introduces a two-state dataset lifecycle system (
Inactive/Active) to make dataset status easier to understand and use across clients, SDKs, and smart contracts.Key Concept: Status reflects data existence, not operational state.
Changes
Smart Contracts
1. Updated
DataSetStatusEnumNotFound,Active,Terminating) to 2 states (Inactive,Active)service_contracts/src/FilecoinWarmStorageService.solBreaking Changes
Enum Changes
NotFoundInactiveActiveActiveTerminatingActiveKey Semantic Changes
Terminated datasets are now
Active(notInactiveorTerminating)pdpEndEpochorisTerminatedto check operational stateStatus reflects data existence, not operational state
Active= "has data"Inactive= "doesn't exist or no data"Closes #169