Land Builtin actors API MVP into next #850

arajasek · 2022-11-16T22:38:59Z

The integration/builtin-api will stay, but will remain on top of master

codecov-commenter · 2022-11-17T01:31:39Z

Codecov Report

Merging #850 (1a434b4) into next (5a4b15b) will decrease coverage by 12.55%.
The diff coverage is 63.13%.

Additional details and impacted files

@@             Coverage Diff             @@
##             next     #850       +/-   ##
===========================================
- Coverage   87.10%   74.54%   -12.56%     
===========================================
  Files         126      127        +1     
  Lines       23278    25940     +2662     
===========================================
- Hits        20276    19338      -938     
- Misses       3002     6602     +3600

Impacted Files	Coverage Δ
actors/market/src/ext.rs	`50.00% <ø> (-50.00%)`	⬇️
actors/paych/src/state.rs	`100.00% <ø> (ø)`
actors/verifreg/src/types.rs	`100.00% <ø> (ø)`
runtime/src/actor_error.rs	`76.74% <ø> (+2.32%)`	⬆️
actors/datacap/src/lib.rs	`52.61% <12.50%> (-24.10%)`	⬇️
actors/verifreg/src/lib.rs	`65.68% <25.00%> (-27.05%)`	⬇️
actors/account/src/lib.rs	`67.18% <50.00%> (-24.31%)`	⬇️
actors/market/src/lib.rs	`62.50% <52.94%> (-22.42%)`	⬇️
actors/power/src/lib.rs	`60.66% <54.90%> (-23.17%)`	⬇️
actors/miner/src/lib.rs	`57.46% <58.91%> (-25.96%)`	⬇️
... and 55 more

arajasek · 2022-11-17T15:50:21Z

Rebased.

arajasek · 2022-11-23T15:27:11Z

Rebased

arajasek · 2022-11-29T19:01:02Z

Rebased

anorth

I think I either reviewed or authored all of this when it first landed in integration/builtin-api, so my review here was fairly cursory.

After landing, we should figure out if the EVM actors should also have restrict_internal_api on invoke.

anorth · 2022-11-29T23:24:19Z

actors/market/src/state.rs

+            format!("failed to load deal state {}", id)
+        })?;
+        Ok(found.cloned())
+    }


FYI @shamb0 for #734

Thanks @anorth, will pick it up.

anorth · 2022-11-29T23:27:16Z

actors/miner/src/types.rs

+impl Cbor for GetAvailableBalanceReturn {}
+
+#[derive(Serialize_tuple, Deserialize_tuple)]
+pub struct GetVestingFundsReturn {


Do we want #[serde(transparent)] here too? Also for GetPeerIDReturn and GetMultiaddrsReturn.

I suggest fixing with a PR to the source branch here, then propagating to next

@anorth We discussed this (at least for GetPeerIDReturn) here. I think we should keep the slightly more complex types (such as GetVestingFundsReturn) to be serialized as tuples.

Ack for PeerID/Multiaddrs.

I don't see logic for complexity of the single embedded object being a deciding factor about whether to wrap it in a tuple or not, tho

Oh, the logic is entirely about consistency with existing types (cf. ComputeDataCommitmentReturn).

ZenGround0 · 2022-11-29T20:36:37Z

actors/account/tests/account_actor_test.rs

    rt.verify();
+
+    // Ok to call exported method number
+    rt.expect_validate_caller_any();


This looks redundant with 113 and on now that we made that part of the test use the exported method

ZenGround0 · 2022-11-30T03:51:41Z

actors/datacap/src/lib.rs

@@ -469,51 +485,51 @@ impl ActorCode for Actor {
                let ret = Self::destroy(rt, cbor::deserialize_params(params)?)?;
                serialize(&ret, "destroy result")
            }
-            Some(Method::Name) => {
+            Some(Method::Name) | Some(Method::NameExported) => {


Why are we keeping the non-exported method numbers active?

Is it a policy decision that built-in actors only call other built-in actors through their internal method numbers? If that's the case, I think it makes sense for uniformity, as otherwise we would see calls to exported and internal method sprinkled over this codebase. However, it might be a usability nit for explorers and other tooling to have two method numbers associated with the same exact behaviour.

ZenGround0 · 2022-11-30T04:09:06Z

actors/market/src/lib.rs

-        // Deal message must have a From field identical to the provider of all the deals.
-        // This allows us to retain and verify only the client's signature in each deal proposal itself.
-        rt.validate_immediate_caller_type(CALLER_TYPES_SIGNABLE.iter())?;
+        rt.validate_immediate_caller_accept_any()?;


Double checking here: the implicit assumption with exposing this is that miner control addresses will be contracts because the caller is constrained to be a miner control address. I want to make sure we are communicating this requirement well because it might be unexpected for users who want to use the storage market. It's possible this is too confusing and we might want to keep PSD from being exported.

Yes, i think you're right, and the suggestion to externalize this information is well-taken. This method now can be called by non-builtin actors, but those actors must still be control addresses.

* 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

* 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

Co-authored-by: zenground0 <[email protected]>

* Market actor: GetDealTermExported: Return (start_epoch, duration) * Market actor: Export getter for deal total price

…#824) * Paych actor: Drop account req, use AuthenticateMessage to verify sigs * Address review * Address review

* 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: 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: Rename AddVerifierClientParams to AddVerifiedClientParams * Verifreg: Export AddVerifiedClient and GetClaims

raulk

Didn't get to all.

raulk · 2022-11-30T12:53:18Z

actors/datacap/src/lib.rs

@@ -469,51 +485,51 @@ impl ActorCode for Actor {
                let ret = Self::destroy(rt, cbor::deserialize_params(params)?)?;
                serialize(&ret, "destroy result")
            }
-            Some(Method::Name) => {
+            Some(Method::Name) | Some(Method::NameExported) => {


Is it a policy decision that built-in actors only call other built-in actors through their internal method numbers? If that's the case, I think it makes sense for uniformity, as otherwise we would see calls to exported and internal method sprinkled over this codebase. However, it might be a usability nit for explorers and other tooling to have two method numbers associated with the same exact behaviour.

raulk · 2022-11-30T12:55:38Z

actors/init/src/lib.rs

        match FromPrimitive::from_u64(method) {
            Some(Method::Constructor) => {
                Self::constructor(rt, cbor::deserialize_params(params)?)?;
                Ok(RawBytes::default())
            }
-            Some(Method::Exec) => {
+            Some(Method::Exec) | Some(Method::ExecExported) => {


@arajasek this is going to require changes in JSON-RPC and CLIs in Lotus. Are we tracking those?

raulk · 2022-11-30T15:31:16Z

actors/miner/tests/change_owner_address_test.rs

+#[test]
+fn change_owner_address_restricted_correctly() {
+    let (h, mut rt) = setup();
+
+    let params = &RawBytes::serialize(NEW_ADDRESS).unwrap();
+    rt.set_caller(make_identity_cid(b"1234"), h.owner);
+
+    // fail to call the unexported method
+
+    expect_abort_contains_message(
+        ExitCode::USR_FORBIDDEN,
+        "must be built-in",
+        rt.call::<Actor>(Method::ChangeOwnerAddress as u64, params),
+    );
+
+    // can call the exported method
+
+    rt.expect_validate_caller_addr(vec![h.owner]);
+    rt.call::<Actor>(Method::ChangeOwnerAddressExported as u64, params).unwrap();
+
+    rt.verify();
+
+    let info = h.get_info(&rt);
+    assert_eq!(h.owner, info.owner);
+    assert_eq!(NEW_ADDRESS, info.pending_owner_address.unwrap());
+
+    // new owner can also call the exported method
+
+    rt.expect_validate_caller_addr(vec![NEW_ADDRESS]);
+    rt.set_caller(make_identity_cid(b"1234"), NEW_ADDRESS);
+    rt.call::<Actor>(Method::ChangeOwnerAddressExported as u64, params).unwrap();
+
+    rt.verify();
+    let info = h.get_info(&rt);
+    assert_eq!(NEW_ADDRESS, info.owner);
+    assert_eq!(NEW_ADDRESS, info.beneficiary);
+    assert!(info.pending_owner_address.is_none());
+
+    h.check_state(&rt);
+}
+


There seems to be some common boilerplate to these tests. Would recommend factoring that out.

arajasek force-pushed the asr/integrate-into-next branch from 13a9860 to 8b4cae8 Compare November 16, 2022 22:56

arajasek mentioned this pull request Nov 16, 2022

Dev -- merge next and api #845

Closed

arajasek force-pushed the asr/integrate-into-next branch from e825d3a to a0e4d75 Compare November 17, 2022 01:03

arajasek force-pushed the asr/integrate-into-next branch from a0e4d75 to 3ec10ab Compare November 17, 2022 15:50

arajasek force-pushed the asr/integrate-into-next branch from 3ec10ab to 52fe937 Compare November 21, 2022 22:31

arajasek changed the title ~~Merge Builtin actors API MVP into next~~ Land Builtin actors API MVP into next Nov 22, 2022

arajasek force-pushed the asr/integrate-into-next branch 2 times, most recently from 750eb93 to 708794d Compare November 23, 2022 15:27

arajasek force-pushed the asr/integrate-into-next branch 2 times, most recently from 406ec0e to 6f2b62b Compare November 28, 2022 23:32

jennijuju requested review from anorth, ZenGround0, raulk and Stebalien November 29, 2022 17:00

arajasek force-pushed the asr/integrate-into-next branch from 1a434b4 to 2404990 Compare November 29, 2022 19:00

anorth approved these changes Nov 29, 2022

View reviewed changes

ZenGround0 reviewed Nov 30, 2022

View reviewed changes

anorth and others added 9 commits November 30, 2022 12:14

Proof of concept exported API for Account actor (#797)

a45d19e

Export stable methods for public access (#807)

2cd742a

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

2993625

Exported API method for market actor escrow/locked balance (#812)

43569bb

Built-in market API for deal proposal metadata (#818)

cd678de

Call exported authenticate method from PSD (#829)

dcb9afa

Co-authored-by: zenground0 <[email protected]>

Drop CALLER_TYPES_SIGNABLE and signable caller validation (#821)

6174e61

Market actor: Minor improvements to two exported getters (#826)

4ba05f3

* Market actor: GetDealTermExported: Return (start_epoch, duration) * Market actor: Export getter for deal total price

anorth and others added 9 commits November 30, 2022 12:16

Exported API for market deal activation state (#819)

76ac0a5

Paych actor: Drop account req, use AuthenticateMessage to verify sigs (…

c39312b

…#824) * Paych actor: Drop account req, use AuthenticateMessage to verify sigs * Address review * Address review

Account actor: Deprecate AuthenticateMessage (#856)

efe7c3c

Market actor: Export PublishStorageDeals (#857)

dc5c6d9

Power: Derive Eq for power types

d3f02cc

Miner: Derive Eq for more types

52a38b4

Power actor: Export methods to CreateMiner and get miner counts (#868)

8b7cbf2

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

d0097ab

* Verifreg: Rename AddVerifierClientParams to AddVerifiedClientParams * Verifreg: Export AddVerifiedClient and GetClaims

arajasek force-pushed the asr/integrate-into-next branch from 2404990 to d0097ab Compare November 30, 2022 18:23

arajasek enabled auto-merge (squash) November 30, 2022 18:23

arajasek disabled auto-merge November 30, 2022 18:23

arajasek enabled auto-merge (rebase) November 30, 2022 18:25

raulk reviewed Nov 30, 2022

View reviewed changes

arajasek merged commit fc7477c into next Nov 30, 2022

arajasek deleted the asr/integrate-into-next branch November 30, 2022 19:16

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

Land Builtin actors API MVP into next #850

Land Builtin actors API MVP into next #850

arajasek commented Nov 16, 2022

codecov-commenter commented Nov 17, 2022 •

edited

Loading

arajasek commented Nov 17, 2022

arajasek commented Nov 23, 2022

arajasek commented Nov 29, 2022

anorth left a comment

anorth Nov 29, 2022

shamb0 Nov 30, 2022

anorth Nov 29, 2022

arajasek Nov 30, 2022

anorth Nov 30, 2022

arajasek Nov 30, 2022

ZenGround0 Nov 29, 2022

ZenGround0 Nov 30, 2022

raulk Nov 30, 2022

anorth Nov 30, 2022

ZenGround0 Nov 30, 2022

arajasek Nov 30, 2022

raulk left a comment

raulk Nov 30, 2022

raulk Nov 30, 2022

raulk Nov 30, 2022

Land Builtin actors API MVP into next #850

Land Builtin actors API MVP into next #850

Conversation

arajasek commented Nov 16, 2022

codecov-commenter commented Nov 17, 2022 • edited Loading

Codecov Report

arajasek commented Nov 17, 2022

arajasek commented Nov 23, 2022

arajasek commented Nov 29, 2022

anorth left a 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

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

raulk left a 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

codecov-commenter commented Nov 17, 2022 •

edited

Loading