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

feat: curio: sectors UI #11869

Merged
merged 74 commits into from
Apr 18, 2024
Merged

feat: curio: sectors UI #11869

merged 74 commits into from
Apr 18, 2024

Conversation

snadrus
Copy link
Contributor

@snadrus snadrus commented Apr 12, 2024

Related Issues

Resolves an item for the Curio beta epic.
Along with 'view', it offers 'terminate'.
image

Proposed Changes

Additional Info

Checklist

Before you mark the PR ready for review, please make sure that:

  • Commits have a clear commit message.
  • PR title is in the form of of <PR type>: <area>: <change being made>
    • example: fix: mempool: Introduce a cache for valid signatures
    • PR type: fix, feat, build, chore, ci, docs, perf, refactor, revert, style, test
    • area, e.g. api, chain, state, market, mempool, multisig, networking, paych, proving, sealing, wallet, deps
  • If the PR affects users (e.g., new feature, bug fix, system requirements change), update the CHANGELOG.md and add details to the UNRELEASED section.
  • New features have usage guidelines and / or documentation updates in
  • Tests exist for new functionality or change in behavior
  • CI is green

Copy link
Contributor

@LexLuthr LexLuthr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest a few changes

  1. We should convert sealed column to PresentOnDisk
  2. Move unsealed next to sealed column
  3. We don't need Fil/Day as the number would be really low to have any useful impact
  4. Seal Proof is not required.

I could not find where we are getting the Size as type Sector{} doesn't seems to have one.

@LexLuthr
Copy link
Contributor

We also need to add another column to UI for "Proving" on chain. A sector can be on-chain but not proving.

@snadrus
Copy link
Contributor Author

snadrus commented Apr 16, 2024

I would suggest a few changes

  1. We should convert sealed column to PresentOnDisk
  2. Move unsealed next to sealed column
  3. We don't need Fil/Day as the number would be really low to have any useful impact
  4. Seal Proof is not required.

I'd like to stick with the has-sealed and has-unsealed language as soon having one will mean that we can (automatically?) generate the other. I want these to seem like 2 sides of the same coin (or a kind of backup) rather than two totally different concepts.
Imagine if everywhere the answer was "false" we could have a top-level button that said "regenerate". Then it would be as simple as

  • "search: has_sealed=false",
  • "select-all"
  • "regenerate"

If the language was totally different between the two, then it would make things more confusing.

@snadrus snadrus requested a review from LexLuthr April 16, 2024 00:21
@snadrus snadrus marked this pull request as ready for review April 16, 2024 00:23
@LexLuthr
Copy link
Contributor

The naming convention sounds good based on your explanation.
I have halted the testing due a functionality bug in storage redeclaration. The sectors missing files are not updated in the DB. It prevents us from gauging the correct behaviour of this change under different scenarios. Let's keep this PR on hold till we have some fix for that bug.

@LexLuthr
Copy link
Contributor

@magik6k Can you test the rendering on your miner? We don't have any good test setup with a lot of sectors.

@LexLuthr
Copy link
Contributor

LexLuthr commented Apr 16, 2024

Apart from adding 3 new columns for Proving, DealWeight and Deals, recent commit also reorders the row based on a hidden column called "Flag". We can flag a sector for any reason like not terminated but local sector is missing or it has expired. We reorder to quickly show SPs if they need to take action for any sector.
Screenshot 2024-04-16 at 5 05 02 PM

@LexLuthr LexLuthr mentioned this pull request Apr 17, 2024
19 tasks
@LexLuthr
Copy link
Contributor

LexLuthr commented Apr 18, 2024

@snadrus Testing results:

  1. Terminate & Delete doesn't work for sector which are not yet on-chain.
  2. We should also check how terminate and remove is handled mid-task.
  3. We need to exclude piece type files when fetching all sector files.

@snadrus
Copy link
Contributor Author

snadrus commented Apr 18, 2024

@snadrus Testing results:

  1. Terminate & Delete doesn't work for sector which are not yet on-chain.
  2. We should also check how terminate and remove is handled mid-task.
  3. We need to exclude piece type files when fetching all sector files.
  1. Not really a problem.
  2. Maybe just not allow it?
  3. How are those getting through today's filter?

@LexLuthr
Copy link
Contributor

@snadrus We need a way to remove sectors safely. I will create a new issue for it.
I have pushed 2 commits which fix all knows issues with Sector UI.
We have 2 pending issues left

  1. OnChainInfo doesn't have deal IDs anymore. If this is post DDO then we can just estimate this from local data.
  2. DDO testing failed as Market RPC has some bug which prevents AP.

@LexLuthr LexLuthr changed the title Feat/sector UI feat: curio: sectors UI Apr 18, 2024
@snadrus snadrus merged commit 6b3e9b1 into master Apr 18, 2024
186 checks passed
@snadrus snadrus deleted the feat/sector-ui branch April 18, 2024 19:57
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 this pull request may close these issues.

None yet

2 participants