Miner actor: Add exported getters for info and monies #811

arajasek · 2022-11-07T19:43:31Z

Adds all-but-one of the new miner methods proposed here.

The missing method is the vesting funds themselves. I'm not sure whether we wanna expose the VestingFunds structure, which is a list of (epoch, amount) tuples. This is probably stable, but I would like a thumbsup before adding it.

codecov-commenter · 2022-11-07T20:11:37Z

Codecov Report

Merging #811 (050427f) into asr/newbuiltin-apis-power (29f7987) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

@@                    Coverage Diff                     @@
##           asr/newbuiltin-apis-power     #811   +/-   ##
==========================================================
  Coverage                      88.13%   88.14%           
==========================================================
  Files                             95       95           
  Lines                          19672    19732   +60     
==========================================================
+ Hits                           17338    17392   +54     
- Misses                          2334     2340    +6

Impacted Files	Coverage Δ
actors/miner/src/lib.rs	`83.61% <100.00%> (+0.35%)`	⬆️
actors/miner/src/types.rs	`97.22% <100.00%> (+0.34%)`	⬆️
actors/miner/src/deadline_info.rs	`79.77% <0.00%> (-16.86%)`	⬇️
runtime/src/util/message_accumulator.rs	`93.63% <0.00%> (-0.91%)`	⬇️
state/src/check.rs	`86.93% <0.00%> (+0.26%)`	⬆️
test_vm/src/lib.rs	`89.37% <0.00%> (+0.34%)`	⬆️
actors/cron/src/lib.rs	`93.33% <0.00%> (+3.33%)`	⬆️

anorth

Having thought more deeply than I did when making that sheet, I think we might be better off exposing a get_available_balance() rather than all the components that go into that. Callers will need to invoke all the bits and recalculate that available amount anyway.

anorth · 2022-11-08T00:02:41Z

actors/miner/src/lib.rs

+    /// Returns the worker address
+    fn get_worker(rt: &mut impl Runtime) -> Result<GetWorkerReturn, ActorError> {
+        rt.validate_immediate_caller_accept_any()?;
+        let state: State = rt.state()?;
+        let worker = get_miner_info(rt.store(), &state)?.worker;
+        Ok(GetWorkerReturn { worker })
+    }
+
+    /// Returns the control addresses
+    fn get_controls(rt: &mut impl Runtime) -> Result<GetControlsReturn, ActorError> {
+        rt.validate_immediate_caller_accept_any()?;
+        let state: State = rt.state()?;
+        let controls = get_miner_info(rt.store(), &state)?.control_addresses;
+        Ok(GetControlsReturn { controls })
+    }


I opened a discussion in Slack about this. We might want to abstract things a bit more and instead add a predicate is_control_address that abstracts over representation of this ACL-type thing.

(Response copied from Slack for visibility)

Re: control addresses, I think I'm onboard with adding an IsControllingAddress method, with a clear definition of what that means in practice.

I do think we need to be careful about adding predicates more generally. It might put us in an awkward position in the future when the underlying model changes. What if the model changes in such a way that some callers of this predicate would want the new answer to be "no", while others would want to treat it as a "yes"?

(in many ways, what we're trying to do in this exercise is engineer good abstract predicates, but it's...hard)

anorth · 2022-11-08T00:08:23Z

actors/miner/src/lib.rs

+        Ok(GetSectorSizeReturn { sector_size })
+    }
+
+    /// Returns the miner's total funds locked as pre_commit_deposit


Suggested change

/// Returns the miner's total funds locked as pre_commit_deposit

/// Returns the miner's total funds locked as pre-commit deposits

anorth · 2022-11-08T00:11:00Z

actors/miner/src/lib.rs

+        Ok(GetPreCommitDepositReturn { pre_commit_deposit: state.pre_commit_deposits })
+    }
+
+    /// Returns the sum of the initial pledge requirements of all active sectors of this miner.


Nit: "active" has a specific meaning in miner state. The jargon you want is "live", but you could also say "committed" or just drop the adjective.

anorth · 2022-11-08T00:12:16Z

actors/miner/src/lib.rs

+
+    /// Returns the absolute value of the locked funds in this miner's vesting table.
+    /// Note that this does NOT include other types of locked funds like precommit deposits.
+    fn get_locked_funds(rt: &mut impl Runtime) -> Result<GetLockedFundsReturn, ActorError> {


Let's rename this to be more descriptive so the comment isn't needed. Maybe get_vesting_funds_total (and we can add get_vesting_funds_table later).

anorth · 2022-11-08T00:12:46Z

actors/miner/src/lib.rs

+    }
+
+    /// Returns the miner's total funds locked as pre_commit_deposit
+    fn get_pre_commit_deposit(


Should we add _total to this and the other methods below? There could later be per-sector versions.

anorth · 2022-11-08T22:32:19Z

actors/miner/src/lib.rs

+        Ok(GetOwnerReturn { owner })
+    }
+
+    /// Returns the worker address


Nit: update the comment

anorth · 2022-11-08T22:32:46Z

actors/miner/src/lib.rs

+        params: IsControllingAddressParam,
+    ) -> Result<IsControllingAddressReturn, ActorError> {
+        rt.validate_immediate_caller_accept_any()?;
+        let state: State = rt.state()?;


Resolve the parameter address to an ID address, as all the ones in state are.

Could also just use ActorID here? There's no meaningful invocation of this with an unresolved address?

anorth · 2022-11-08T22:36:57Z

actors/miner/tests/exported_getters.rs

+        .call::<Actor>(Method::GetAvailableBalanceExported as u64, &RawBytes::default())
+        .unwrap()
+        .deserialize()
+        .unwrap();


I think it would be reasonable, perhaps even better, to fold this into existing tests. Some of them must already check balance components etc. Add this to the harness and then call it from sector tests (possibly replacing some other balance checks).

Yeah, I like what you did with the market test.

* 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

* 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>

* 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

* 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) * Miner: Export method to GetPendingOwner * MarketNotifyDeal (#944) Co-authored-by: zenground0 <ZenGround0@users.noreply.github.com> * Verifreg: Call AuthenticateMessage instead of validating siggys * Multisig: Do not export any functionality (#967) * use const for EX_DEAL_EXPIRED (#954) * Address review 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> Co-authored-by: Shashank <99187193+sudo-shashank@users.noreply.github.com>

* 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 (filecoin-project#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

…ecoin-project#1004) * Proof of concept exported API for Account actor (filecoin-project#797) * Export stable methods for public access (filecoin-project#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 (filecoin-project#809) * Exported API method for market actor escrow/locked balance (filecoin-project#812) * Power actor: Add exported getters for raw power (filecoin-project#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 (filecoin-project#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 (filecoin-project#818) * Call exported authenticate method from PSD (filecoin-project#829) Co-authored-by: zenground0 <ZenGround0@users.noreply.github.com> * Drop CALLER_TYPES_SIGNABLE and signable caller validation (filecoin-project#821) * Market actor: Minor improvements to two exported getters (filecoin-project#826) * Market actor: GetDealTermExported: Return (start_epoch, duration) * Market actor: Export getter for deal total price * Exported API for market deal activation state (filecoin-project#819) * Paych actor: Drop account req, use AuthenticateMessage to verify sigs (filecoin-project#824) * Paych actor: Drop account req, use AuthenticateMessage to verify sigs * Address review * Address review * Account actor: Deprecate AuthenticateMessage (filecoin-project#856) * Market actor: Export PublishStorageDeals (filecoin-project#857) * Miner: Export several more methods (filecoin-project#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 (filecoin-project#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 (filecoin-project#873) * Verifreg: Rename AddVerifierClientParams to AddVerifiedClientParams * Verifreg: Export AddVerifiedClient and GetClaims * Datacap actor: Modify exported methods (filecoin-project#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 (filecoin-project#910) * Miner: Export method to GetPendingOwner * MarketNotifyDeal (filecoin-project#944) Co-authored-by: zenground0 <ZenGround0@users.noreply.github.com> * Verifreg: Call AuthenticateMessage instead of validating siggys * Multisig: Do not export any functionality (filecoin-project#967) * use const for EX_DEAL_EXPIRED (filecoin-project#954) * Address review 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> Co-authored-by: Shashank <99187193+sudo-shashank@users.noreply.github.com>

arajasek force-pushed the asr/newbuiltin-apis-power branch from 29f7987 to 1e022c6 Compare November 7, 2022 20:41

arajasek force-pushed the asr/newbuiltin-apis-miner branch 2 times, most recently from bde4191 to 33fa947 Compare November 7, 2022 22:26

anorth reviewed Nov 8, 2022

View reviewed changes

anorth approved these changes Nov 8, 2022

View reviewed changes

arajasek added 8 commits November 8, 2022 17:51

Miner actor: Add exported getters for info and monies

903abdd

Tweak comment

bf83998

Miner actor: Replace GetWorker and GetControls with IsControllingAddress

374e02e

Miner actor: Add exported GetAvailableBalance

72ed577

Miner actor: Add exported GetVestingFunds

e6390f7

Miner actor: Remove exported monies getters

721efaf

Miner actor: types: serialize one-element structs transparently

11b9608

Address review

e321d71

arajasek force-pushed the asr/newbuiltin-apis-miner branch from a74a9fe to e321d71 Compare November 8, 2022 23:28

arajasek merged commit 6e7f320 into asr/newbuiltin-apis-power Nov 8, 2022

arajasek deleted the asr/newbuiltin-apis-miner branch November 8, 2022 23:49

jennijuju added this to the Reenginering built-in actors API milestone Dec 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Miner actor: Add exported getters for info and monies #811

Miner actor: Add exported getters for info and monies #811

arajasek commented Nov 7, 2022 •

edited

Loading

codecov-commenter commented Nov 7, 2022

anorth left a comment

anorth Nov 8, 2022

arajasek Nov 8, 2022 •

edited

Loading

anorth Nov 8, 2022

anorth Nov 8, 2022

anorth Nov 8, 2022

anorth Nov 8, 2022

anorth Nov 8, 2022

anorth Nov 8, 2022

arajasek Nov 8, 2022

anorth Nov 8, 2022

arajasek Nov 8, 2022

	/// Returns the miner's total funds locked as pre_commit_deposit
	/// Returns the miner's total funds locked as pre-commit deposits

Miner actor: Add exported getters for info and monies #811

Miner actor: Add exported getters for info and monies #811

Conversation

arajasek commented Nov 7, 2022 • edited Loading

codecov-commenter commented Nov 7, 2022

Codecov Report

anorth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arajasek Nov 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arajasek commented Nov 7, 2022 •

edited

Loading

arajasek Nov 8, 2022 •

edited

Loading