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

Delete deprecated UseBytes and RestoreBytes methods from verified registry #658

Merged
merged 1 commit into from
Sep 16, 2022

Conversation

anorth
Copy link
Member

@anorth anorth commented Sep 15, 2022

Deleting code is the best.

  • Rebase on decouple-fil+ after base PR is merged.

@anorth anorth requested a review from ZenGround0 September 15, 2022 02:43
@codecov-commenter
Copy link

Codecov Report

Merging #658 (e35ffb8) into anorth/market-allocation (0435948) will increase coverage by 0.15%.
The diff coverage is n/a.

Impacted file tree graph

@@                     Coverage Diff                      @@
##           anorth/market-allocation     #658      +/-   ##
============================================================
+ Coverage                     84.45%   84.60%   +0.15%     
============================================================
  Files                            95       95              
  Lines                         19054    18989      -65     
============================================================
- Hits                          16092    16066      -26     
+ Misses                         2962     2923      -39     
Impacted Files Coverage Δ
actors/verifreg/src/lib.rs 91.58% <ø> (+3.03%) ⬆️
actors/verifreg/src/types.rs 92.00% <ø> (-0.31%) ⬇️
actors/miner/src/lib.rs 80.35% <0.00%> (-0.19%) ⬇️
test_vm/src/lib.rs 88.99% <0.00%> (+0.23%) ⬆️
actors/miner/src/deadline_info.rs 96.62% <0.00%> (+16.85%) ⬆️

@@ -790,76 +786,4 @@ mod datacap {
}
h.check_state(&rt);
}

#[test]
fn restore() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I have a feeling we'll want to reinstate these tests for verifreg::RemoveExpiredAllocations. A follow up is fine but in that case maybe leave these tests commented out so review is easier?

Copy link
Member Author

Choose a reason for hiding this comment

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

I looked through and I think they'll be different enough to need basically re-writing. It's a good point to come check here for reference though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Looking through these now while finishing off testing:

  • We use actor IDs for the client/provider, so no longer need to resolve addresses
  • Anyone can call RemoveExpiredAllocations, not just the market actor
  • No need to check minimums since we're dealing with allocation records the verifreg already has

Base automatically changed from anorth/market-allocation to decouple-fil+ September 15, 2022 23:59
@anorth anorth force-pushed the anorth/verifreg-purge branch from e35ffb8 to 4bd98ed Compare September 15, 2022 23:59
@anorth anorth merged commit 16b923a into decouple-fil+ Sep 16, 2022
@anorth anorth deleted the anorth/verifreg-purge branch September 16, 2022 00:29
shamb0 pushed a commit to shamb0/builtin-actors that referenced this pull request Jan 31, 2023
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.

3 participants