Remove the market actor state mutation pattern #734

shamb0 · 2022-10-04T10:00:40Z

Fixes #656 | Remove the market actor state mutation pattern

Took state mutation pattern of fil_actor_miner as reference,
and modified the fil_actor_market mutation pattern. following things are verified on proposed patch & ready for review.

State mutation refactor on state.rs - Done
Integration to struct Actor - Done
Status of existing Unittest - all OK

Signed-off-by: shamb0 <r.raajey@gmail.com>

…-refactor-actor-market-state-mutation

codecov-commenter · 2022-10-04T10:23:52Z

Codecov Report

Merging #734 (7e954fa) into master (890ed9f) will increase coverage by 0.49%.
The diff coverage is 94.01%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #734      +/-   ##
==========================================
+ Coverage   88.20%   88.69%   +0.49%     
==========================================
  Files          95       95              
  Lines       19628    19650      +22     
==========================================
+ Hits        17312    17429     +117     
+ Misses       2316     2221      -95

Impacted Files	Coverage Δ
actors/market/src/lib.rs	`89.98% <91.75%> (+5.07%)`	⬆️
actors/market/src/state.rs	`92.58% <94.41%> (+8.90%)`	⬆️
actors/power/src/lib.rs	`83.62% <0.00%> (-0.65%)`	⬇️
actors/datacap/src/lib.rs	`76.30% <0.00%> (-0.35%)`	⬇️
state/src/check.rs	`86.66% <0.00%> (-0.27%)`	⬇️
test_vm/src/lib.rs	`88.91% <0.00%> (-0.12%)`	⬇️

anorth · 2022-10-04T18:56:43Z

Thanks for rebasing this. We'll take a look in a week or so after we're confident that nv17 has settled on master

shamb0 · 2022-10-05T06:58:16Z

Thanks @anorth,

One observation from my perspective is ...

Storage of state instances, gets loaded frequently.
wondering about the overhead cost associated with it.

In current patch, this overhead is due to, most of the states
are muted inside the loop context, I hope this can be
refactored and optimized in better way.

To make review simpler, Can I take the optimization
as separate PR ?

anorth · 2022-10-06T21:56:03Z

Yes please defer any optimisations to a later PR. This one should be as much as possible a mechanical change.

Signed-off-by: shamb0 <r.raajey@gmail.com>

…-refactor-actor-market-state-mutation

anorth

This looks great. Nice batching of updates – thanks.

I'm so sorry this took so long to review. Would you please rebase and update for feedback, and then I will make sure to re-review quickly so we can land this.

cc @ZenGround0 @arajasek

actors/market/src/lib.rs

actors/market/src/state.rs

Signed-off-by: shamb0 <r.raajey@gmail.com>

anorth

Awesome. I've a few naming nits, but otherwise this looks great. I want to give an opportunity for @ZenGround0 and/or @arajasek to skim this before merging, but LGTM.

actors/market/src/lib.rs

actors/market/src/state.rs

arajasek

Thanks, this looks great -- just a couple nits.

actors/market/src/state.rs

actors/market/src/lib.rs

…-refactor-actor-market-state-mutation

Co-authored-by: Alex <445306+anorth@users.noreply.github.com>

Signed-off-by: shamb0 <r.raajey@gmail.com>

shamb0 · 2022-12-05T13:06:14Z

Thanks @anorth & @arajasek, All the nits are closed now.
PR is in-sync with latest master branch commits & ready for intake.

* Remove the market actor state mutation pattern (#734) Fixes #656 * Proof of concept exported API for Account actor (#797) * Export stable methods for public access (#807) * Export Datacap Actor methods * Export Init Actor methods * Export Market Actor methods * Export Miner Actor methods * Export Multisig Actor methods * Export Verifreg Actor methods * Address review * Restrict internal APIs of all actors (#809) * Exported API method for market actor escrow/locked balance (#812) * Power actor: Add exported getters for raw power (#810) * Power actor: Add exported getters for raw power * FRC-XXXX is FRC-0042 * Power actor: network_raw_power: Return this_epoch_raw_byte_power * Power actor: miner_raw_power: Return whether above consensus min power * Power actor: types: serialize one-element structs transparently * Address review * Miner actor: Add exported getters for info and monies (#811) * Miner actor: Add exported getters for info and monies * Tweak comment * Miner actor: Replace GetWorker and GetControls with IsControllingAddress * Miner actor: Add exported GetAvailableBalance * Miner actor: Add exported GetVestingFunds * Miner actor: Remove exported monies getters * Miner actor: types: serialize one-element structs transparently * Address review * Address review * Built-in market API for deal proposal metadata (#818) * Call exported authenticate method from PSD (#829) Co-authored-by: zenground0 <ZenGround0@users.noreply.github.com> * Drop CALLER_TYPES_SIGNABLE and signable caller validation (#821) * Market actor: Minor improvements to two exported getters (#826) * Market actor: GetDealTermExported: Return (start_epoch, duration) * Market actor: Export getter for deal total price * Exported API for market deal activation state (#819) * Paych actor: Drop account req, use AuthenticateMessage to verify sigs (#824) * Paych actor: Drop account req, use AuthenticateMessage to verify sigs * Address review * Address review * Account actor: Deprecate AuthenticateMessage (#856) * Market actor: Export PublishStorageDeals (#857) * Miner: Export several more methods (#863) * Miner: Export ChangeWorkerAddress * Miner: Export ChangePeerID * Miner: Export WithdrawBalance * Miner: Export ChangeMultiaddrs * Miner: Export ConfirmUpdateWorkerKey * Miner: Export RepayDebt * Miner: Export ChangeOwnerAddress * Miner: Add exported getters for PeerID & multiaddrs * Miner: Refactor: Rename ConfirmUpdateWorkerKey to ConfirmChangeWorkerAddress * Power actor: Export methods to CreateMiner and get miner counts (#868) * Power: Export CreateMiner * Power Actor: Export MinerCount and MinerConsensusCount * Update actors/power/src/lib.rs Co-authored-by: Alex <445306+anorth@users.noreply.github.com> Co-authored-by: Alex <445306+anorth@users.noreply.github.com> * Verifreg: Export AddVerifiedClient and GetClaims (#873) * Verifreg: Rename AddVerifierClientParams to AddVerifiedClientParams * Verifreg: Export AddVerifiedClient and GetClaims * Datacap actor: Modify exported methods (#909) * Datacap: Export Mint and Destroy * Datacap actor: Deprecate all internal methods * Datacap actor: Rename BalanceOf to Balance * Datacap actor: Add Granularity method * fix: comments on newly exported methods (#910) Co-authored-by: RK <r.raajey@gmail.com> Co-authored-by: Alex <445306+anorth@users.noreply.github.com> Co-authored-by: ZenGround0 <5515260+ZenGround0@users.noreply.github.com> Co-authored-by: zenground0 <ZenGround0@users.noreply.github.com>

Fixes filecoin-project#656

shamb0 added 2 commits October 4, 2022 15:26

state mutation refactor

970d918

Signed-off-by: shamb0 <r.raajey@gmail.com>

Merge remote-tracking branch 'upstream/master' into shamb0/rev-v2-656…

430527a

…-refactor-actor-market-state-mutation

shamb0 added 3 commits October 7, 2022 21:08

make clippy happy

071350a

Signed-off-by: shamb0 <r.raajey@gmail.com>

revert back bad fix

37de6f1

Signed-off-by: shamb0 <r.raajey@gmail.com>

Merge remote-tracking branch 'upstream/master' into shamb0/rev-v2-656…

7a533c7

…-refactor-actor-market-state-mutation

anorth reviewed Nov 28, 2022

View reviewed changes

anorth mentioned this pull request Nov 29, 2022

Land Builtin actors API MVP into next #850

Merged

shamb0 added 2 commits December 1, 2022 10:14

merge to upstream master & review comments closure

3c4e0a5

Signed-off-by: shamb0 <r.raajey@gmail.com>

function naming refactor

10e0c98

Signed-off-by: shamb0 <r.raajey@gmail.com>

anorth approved these changes Dec 1, 2022

View reviewed changes

anorth changed the title ~~#656 Fix | actor market | refactor state mutation #726~~ Remove the market actor state mutation pattern Dec 1, 2022

arajasek approved these changes Dec 1, 2022

View reviewed changes

actors/market/src/state.rs Show resolved Hide resolved

actors/market/src/lib.rs Outdated Show resolved Hide resolved

shamb0 and others added 5 commits December 3, 2022 15:31

Merge remote-tracking branch 'upstream/master' into shamb0/rev-v2-656…

872db61

…-refactor-actor-market-state-mutation

Update actors/market/src/state.rs

560558f

Co-authored-by: Alex <445306+anorth@users.noreply.github.com>

review closure & function naming refactor

b95b093

Signed-off-by: shamb0 <r.raajey@gmail.com>

review closure & function naming refactor

25c67cf

Signed-off-by: shamb0 <r.raajey@gmail.com>

review closure & function naming refactor

7e954fa

Signed-off-by: shamb0 <r.raajey@gmail.com>

anorth merged commit dc8077c into filecoin-project:master Dec 5, 2022

shamb0 added a commit to shamb0/builtin-actors that referenced this pull request Jan 31, 2023

Remove the market actor state mutation pattern (filecoin-project#734)

c260586

Fixes filecoin-project#656

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove the market actor state mutation pattern #734

Remove the market actor state mutation pattern #734

shamb0 commented Oct 4, 2022

codecov-commenter commented Oct 4, 2022 •

edited

Loading

anorth commented Oct 4, 2022

shamb0 commented Oct 5, 2022

anorth commented Oct 6, 2022

anorth left a comment

anorth left a comment

arajasek left a comment

shamb0 commented Dec 5, 2022

Remove the market actor state mutation pattern #734

Remove the market actor state mutation pattern #734

Conversation

shamb0 commented Oct 4, 2022

codecov-commenter commented Oct 4, 2022 • edited Loading

Codecov Report

anorth commented Oct 4, 2022

shamb0 commented Oct 5, 2022

anorth commented Oct 6, 2022

anorth left a comment

Choose a reason for hiding this comment

anorth left a comment

Choose a reason for hiding this comment

arajasek left a comment

Choose a reason for hiding this comment

shamb0 commented Dec 5, 2022

codecov-commenter commented Oct 4, 2022 •

edited

Loading