Power actor: Add exported getters for raw power #810

arajasek · 2022-11-07T17:16:21Z

Adds the new power methods proposed here.

arajasek · 2022-11-07T18:10:33Z

actors/power/src/types.rs

@@ -65,3 +66,26 @@ pub struct CurrentTotalPowerReturn {
    pub pledge_collateral: TokenAmount,
    pub quality_adj_power_smoothed: FilterEstimate,
 }
+
+#[derive(Serialize_tuple, Deserialize_tuple)]


How do we want to serialize these? Should they be serialized as structs (as currently written), or their inner type (StoragePower / Address)?

I would have said bare inner types, but above have suggested adding a second member to each of these.

Discussed offline: We're going to maintain existing convention, which is for single-element structs to be serialized transparently.

Agree with the result. I personally would skip the struct and just return StoragePower directly, but I'm not adamant. If we use the struct, we should go add them to all the other cases where we return a raw value.

I do think I prefer to have an explicit struct. We're already inconsistent about whether we do this (eg. Market::WithdrawBalanceReturn, vs. Miner::change_owner_address).

I'll do a quick audit and change any cases I find, but I'd rather not be especially thorough.

Actually, this should be a separate PR regardless. #817

arajasek · 2022-11-07T20:41:31Z

Rebased on integration/builtin-api

actors/power/src/lib.rs

anorth · 2022-11-07T22:26:37Z

actors/power/src/lib.rs

+        rt.validate_immediate_caller_accept_any()?;
+        let st: State = rt.state()?;
+
+        Ok(NetworkRawPowerReturn { raw_byte_power: st.total_raw_byte_power })


We also have a version of this frozen at the end of the last epoch. It's used by the built-in market actor in order to remove any advantage to re-ordering transactions. If I were to choose just one of the two, I think the "this epoch" version might be better to expose. I think it's reasonable to commit to this behaviour for the future, even if the built-in market stops using it or stops being built-in.

We could also expose both the stable version and instantaneous.

I yo-yoed on this, and went with the unfrozen number because it was the more intuitive one. I didn't feel strongly, though, so I'm going to switch to the stable version on your suggestion.

I would be pretty opposed to exposing both, I worry that would only confuse.

anorth · 2022-11-07T22:33:51Z

actors/power/src/lib.rs

+        Ok(NetworkRawPowerReturn { raw_byte_power: st.total_raw_byte_power })
+    }
+
+    /// Returns the raw power of the specified miner.


Perhaps use the term "claimed" by the miner?

Note in docs that this does not include calculation of whether the miner meeds the minimum commitment to have any consensus power. The sum of miner_raw_power for all miners does not equal network_raw_power for this reason.

I think we should probably expose both here. See the method miner_nominal_power_meets_consensus_minimum in state.rs. We can return(claimed-power, meets-consensus-minimum?). Then we should have network_raw_power equal to the sum of miner power where the bool is true.

Yeah, I think I'm happy to go with returning both. I am a little unsure how useful it is to share meets-consensus-minimum, but it's worth doing.

anorth · 2022-11-07T22:35:11Z

actors/power/src/types.rs

@@ -65,3 +66,26 @@ pub struct CurrentTotalPowerReturn {
    pub pledge_collateral: TokenAmount,
    pub quality_adj_power_smoothed: FilterEstimate,
 }
+
+#[derive(Serialize_tuple, Deserialize_tuple)]


I would have said bare inner types, but above have suggested adding a second member to each of these.

arajasek · 2022-11-08T17:10:18Z

@anorth Thanks for the review! I've added four short commits that should address your feedback.

actors/power/src/lib.rs

actors/power/src/state.rs

anorth · 2022-11-08T22:12:35Z

actors/power/src/types.rs

@@ -65,3 +66,26 @@ pub struct CurrentTotalPowerReturn {
    pub pledge_collateral: TokenAmount,
    pub quality_adj_power_smoothed: FilterEstimate,
 }
+
+#[derive(Serialize_tuple, Deserialize_tuple)]


Agree with the result. I personally would skip the struct and just return StoragePower directly, but I'm not adamant. If we use the struct, we should go add them to all the other cases where we return a raw value.

actors/power/src/types.rs

anorth · 2022-11-08T22:18:55Z

actors/power/src/types.rs

+#[derive(Serialize_tuple, Deserialize_tuple)]
+#[serde(transparent)]
+pub struct MinerRawPowerParams {
+    pub miner: Address,


You didn't resolve this address, so it'll only work as an ID address. This raises another API design question: Addresses or actor IDs?

Discussed offline: Actor IDs.

codecov-commenter · 2022-11-08T23:14:57Z

Codecov Report

❗ No coverage uploaded for pull request base (integration/builtin-api@020fa96). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head 6e7f320 differs from pull request most recent head 32065ce. Consider uploading reports for the commit 32065ce to get more accurate results

Additional details and impacted files

@@                    Coverage Diff                     @@
##             integration/builtin-api     #810   +/-   ##
==========================================================
  Coverage                           ?   88.16%           
==========================================================
  Files                              ?       95           
  Lines                              ?    19748           
  Branches                           ?        0           
==========================================================
  Hits                               ?    17410           
  Misses                             ?     2338           
  Partials                           ?        0

* 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

anorth · 2022-11-08T23:49:23Z

actors/power/src/state.rs

-        miner: &Address,
-    ) -> anyhow::Result<bool> {
+        miner: ActorID,
+    ) -> anyhow::Result<(StoragePower, bool)> {


Sorry, I was vague earlier. Change this to return Result<(..), ActorError>. You'll then be able to specify different exit codes at the ? operators below (and just ? up at the call site).

anorth · 2022-11-09T00:12:04Z

actors/power/src/state.rs

+                ExitCode::USR_ILLEGAL_STATE,
+                format!("failed to get claim for miner: {}", miner),
+            )?
+            .ok_or_else(|| {


You can use with_context_code on Option too, but either way is fine.

anorth · 2022-11-09T00:12:17Z

actors/power/src/state.rs

-            .ok_or_else(|| anyhow!("no claim for actor: {}", miner))?;
+    ) -> Result<(StoragePower, bool), ActorError> {
+        let claims = make_map_with_root_and_bitwidth(&self.claims, s, HAMT_BIT_WIDTH)
+            .context_code(


Nit: use with_context_code to avoid the format! in success cases.

* 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 <[email protected]> * 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 <[email protected]> Co-authored-by: Alex <[email protected]> * 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 <[email protected]> Co-authored-by: Alex <[email protected]> Co-authored-by: ZenGround0 <[email protected]> Co-authored-by: zenground0 <[email protected]>

* 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 <[email protected]> * 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 <[email protected]> Co-authored-by: Alex <[email protected]> * 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 <[email protected]> * 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 <[email protected]> Co-authored-by: ZenGround0 <[email protected]> Co-authored-by: zenground0 <[email protected]> Co-authored-by: Shashank <[email protected]>

* 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 <[email protected]> * 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 <[email protected]> Co-authored-by: Alex <[email protected]> * 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 <[email protected]> * 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 <[email protected]> Co-authored-by: ZenGround0 <[email protected]> Co-authored-by: zenground0 <[email protected]> Co-authored-by: Shashank <[email protected]>

arajasek force-pushed the asr/newbuiltin-apis-power branch from c2891ec to 29f7987 Compare November 7, 2022 17:49

arajasek commented Nov 7, 2022

View reviewed changes

arajasek mentioned this pull request Nov 7, 2022

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

Merged

Base automatically changed from asr/restrict-all to integration/builtin-api November 7, 2022 20:39

Power actor: Add exported getters for raw power

1e022c6

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

anorth reviewed Nov 7, 2022

View reviewed changes

arajasek added 4 commits November 8, 2022 09:58

FRC-XXXX is FRC-0042

5ef2f1d

Power actor: network_raw_power: Return this_epoch_raw_byte_power

366113f

Power actor: miner_raw_power: Return whether above consensus min power

f706d7d

Power actor: types: serialize one-element structs transparently

2ab92f7

Merge branch 'integration/builtin-api' into asr/newbuiltin-apis-power

08097b1

anorth reviewed Nov 8, 2022

View reviewed changes

Address review

6096661

arajasek mentioned this pull request Nov 8, 2022

Add types for one-value params and returns #817

Closed

anorth approved these changes Nov 8, 2022

View reviewed changes

anorth approved these changes Nov 9, 2022

View reviewed changes

Address review

78870b9

arajasek force-pushed the asr/newbuiltin-apis-power branch from 32065ce to 78870b9 Compare November 9, 2022 14:22

arajasek merged commit 245de89 into integration/builtin-api Nov 9, 2022

arajasek deleted the asr/newbuiltin-apis-power branch November 9, 2022 14:23

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

Power actor: Add exported getters for raw power #810

Power actor: Add exported getters for raw power #810

arajasek commented Nov 7, 2022 •

edited

Loading

arajasek Nov 7, 2022

anorth Nov 7, 2022

arajasek Nov 8, 2022

anorth Nov 8, 2022

arajasek Nov 8, 2022

arajasek Nov 8, 2022

arajasek commented Nov 7, 2022

anorth Nov 7, 2022

arajasek Nov 8, 2022

anorth Nov 7, 2022

arajasek Nov 8, 2022

anorth Nov 7, 2022

arajasek commented Nov 8, 2022

anorth Nov 8, 2022

anorth Nov 8, 2022

arajasek Nov 8, 2022

codecov-commenter commented Nov 8, 2022 •

edited

Loading

anorth Nov 8, 2022

anorth Nov 9, 2022

anorth Nov 9, 2022

Power actor: Add exported getters for raw power #810

Power actor: Add exported getters for raw power #810

Conversation

arajasek commented Nov 7, 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

arajasek commented Nov 7, 2022

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 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Nov 8, 2022 • edited Loading

Codecov Report

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

codecov-commenter commented Nov 8, 2022 •

edited

Loading