Use Option<IpldBlock> for all message params #913

arajasek · 2022-12-05T23:23:56Z

#758 (the "params" half)

This is a mostly-manual overhaul of the system to use Option for message params, as driven by filecoin-project/ref-fvm#1199. As part of it, we start using the macros introduced in #947 for the dispatch table for all actors.

Other changes:

The ReceiverHooks of the account & multisig actors now explicitly receive UniversalReceiverParams, where previously they accepted anything. They will fail if called with input that isn't validly serialized UniversalReceiverParams (though the value of the UniversalReceiverParams itself doesn't matter)
Methods that take no input previously succeeded no matter what was passed to them. They now explicitly require "no input". The exception is Send.

arajasek · 2022-12-08T01:46:03Z

This is now ready for review. The GH dependencies will be dropped when the FVM PR lands, but I'd like confidence that this is the right direction before landing work in the FVM.

anorth

Thanks for taking this on, but this PR is too large to review. It's doing too many things at once, unnecessarily. I gotta put my foot down somewhere, and I'm doing it here. Please engage me on Slack or similar for discussion about why large PRs are very costly.

I've looked at about a third of this. It looks directionally fine. I can review more of it on a second pass.

A bunch of things that are not just what this PR title says should be broken out to separate PRs, to land ahead of this one.

Removing use of Cbor trait
Upgrading frc46_token with all changes except this IpldBlock change
Fancy but complex macros to reduce dispatch boilerplate
Changing send to use IpldBlock too

actors/account/tests/account_actor_test.rs

anorth · 2022-12-11T23:08:20Z

actors/datacap/src/lib.rs

@@ -71,14 +72,14 @@ pub struct Actor;

 impl Actor {
    /// Constructor for DataCap Actor
-    pub fn constructor(rt: &mut impl Runtime, governor: Address) -> Result<(), ActorError> {
+    pub fn constructor(rt: &mut impl Runtime, params: Address) -> Result<(), ActorError> {


This rename reduces clarity and doesn't seem necessary

Sure. I was hoping to have all methods uniformly take a single parameter named params (or no parameters at all), but I'm not fussed.

I think params is fine if we've wrapped in a transparent struct, so the struct member then has an informative name.

actors/datacap/src/testing.rs

runtime/src/actor_error.rs

runtime/src/dispatch.rs

rust-toolchain

anorth · 2022-12-11T23:32:47Z

A further suggestion to make this more manageable is to apply it to only one or two simple actors initially, while figuring out all the supporting code, then do the mechanical application to the rest at the end.

codecov-commenter · 2022-12-12T19:05:34Z

Codecov Report

Merging #913 (d5d2212) into master (45c56ed) will increase coverage by 0.21%.
The diff coverage is 99.36%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #913      +/-   ##
==========================================
+ Coverage   88.84%   89.05%   +0.21%     
==========================================
  Files          92       92              
  Lines       19584    19320     -264     
==========================================
- Hits        17399    17206     -193     
+ Misses       2185     2114      -71

Impacted Files	Coverage Δ
actors/power/src/ext.rs	`83.33% <ø> (ø)`
runtime/src/actor_error.rs	`81.05% <ø> (ø)`
runtime/src/lib.rs	`88.00% <ø> (ø)`
test_vm/src/lib.rs	`88.97% <93.75%> (-0.54%)`	⬇️
actors/account/src/lib.rs	`91.66% <100.00%> (+0.17%)`	⬆️
actors/cron/src/lib.rs	`94.11% <100.00%> (-2.44%)`	⬇️
actors/datacap/src/lib.rs	`77.82% <100.00%> (+1.26%)`	⬆️
actors/init/src/lib.rs	`97.87% <100.00%> (+1.57%)`	⬆️
actors/market/src/lib.rs	`89.74% <100.00%> (-0.26%)`	⬇️
actors/miner/src/lib.rs	`83.12% <100.00%> (-0.30%)`	⬇️
... and 32 more

arajasek · 2022-12-14T16:02:02Z

Lowering to draft until pre-factors have landed and conflicts resolved

arajasek · 2022-12-15T02:25:56Z

actors/datacap/src/lib.rs

-    where
-        RT: Runtime,
-    {
-        // I'm trying to find a fixed template for these blocks so we can macro it.


arajasek · 2022-12-15T15:53:29Z

Rebased.

actors/power/src/lib.rs

actors/power/src/types.rs

actors/reward/src/types.rs

actors/verifreg/src/lib.rs

runtime/src/runtime/fvm.rs

actors/miner/src/lib.rs

anorth · 2022-12-15T21:06:57Z

Cargo.toml

@@ -53,6 +53,7 @@ members = [
     "test_vm",
 ]

+[patch.crates-io]


Nit: revert

anorth · 2022-12-15T21:11:09Z

actors/paych/src/lib.rs

@@ -159,8 +161,14 @@ impl Actor {
        }

        if let Some(extra) = &sv.extra {
-            rt.send(&extra.actor, extra.method, extra.data.clone(), TokenAmount::zero())
-                .map_err(|e| e.wrap("spend voucher verification failed"))?;
+            // TODO: Is this the best way to handle this?


This comment isn't helpful. Please either explain the design issue, or remove.

anorth · 2022-12-15T21:13:42Z

actors/verifreg/src/lib.rs

@@ -68,12 +69,12 @@ pub struct Actor;

 impl Actor {
    /// Constructor for Registry Actor
-    pub fn constructor(rt: &mut impl Runtime, root_key: Address) -> Result<(), ActorError> {
+    pub fn constructor(rt: &mut impl Runtime, params: Address) -> Result<(), ActorError> {


Nit: I don't think this rename is helpful

anorth · 2022-12-15T21:15:50Z

test_vm/src/lib.rs

@@ -371,11 +372,7 @@ impl<'bs> VM<'bs> {

    pub fn get_state<C: Cbor>(&self, addr: Address) -> Option<C> {


Re-opening #700 until we remove use of Cbor here too

ZenGround0 · 2022-12-15T22:26:44Z

Cargo.lock

@@ -2168,3 +2188,60 @@ name = "winapi-x86_64-pc-windows-gnu"
 version = "0.4.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "712e227841d057c1ee1cd2fb22fa7e5a5461ae8e48fa2ca79ec42cfc1931183f"
+
+[[package]]
+name = "windows-sys"


Is introducing windows stuff required for this PR? That's unexpected

actors/account/tests/account_actor_test.rs

ZenGround0 · 2022-12-15T23:43:55Z

actors/account/tests/account_actor_test.rs

@@ -117,7 +128,7 @@ fn authenticate_message() {
        plaintext: vec![],
        result: Ok(()),
    });
-    assert_eq!(RawBytes::default(), rt.call::<AccountActor>(3, &params).unwrap());
+    assert_eq!(RawBytes::default(), rt.call::<AccountActor>(3, params.clone()).unwrap());


I would have thought the None would carry over to the output of call?

Not yet -- returns will be a separate PR.

ZenGround0 · 2022-12-15T23:53:50Z

actors/market/src/lib.rs

@@ -86,7 +87,7 @@ impl Actor {
    }

    /// Deposits the received value into the balance held in escrow.
-    fn add_balance(rt: &mut impl Runtime, provider_or_client: Address) -> Result<(), ActorError> {
+    fn add_balance(rt: &mut impl Runtime, params: Address) -> Result<(), ActorError> {


nit: Overall I'm not convinced standardizing params names is the best way. Not a super strong opinion though.

that's the majority opinion, so I'm gonna revert

ZenGround0 · 2022-12-15T23:55:30Z

actors/miner/src/lib.rs

-                             // TODO: when landing https://github.com/filecoin-project/builtin-actors/pull/518
-                             // ExtendProofValidity
+    ExtendCommittmentLegacy,
+    // handle only legacy sectors


undo, if you want this above a field it should be above the ExtendCommittmentLegacy field

ZenGround0 · 2022-12-16T00:05:21Z

actors/power/src/lib.rs

@@ -289,7 +292,7 @@ impl Actor {

    fn submit_porep_for_bulk_verify(
        rt: &mut impl Runtime,
-        seal_info: SealVerifyInfo,


Reiterating: I think it makes sense for methods to define param names that make sense in context

arajasek mentioned this pull request Dec 5, 2022

Use Option<IpldBlock> for all message params #871

Closed

5 tasks

arajasek force-pushed the asr/ipld-block-master branch 4 times, most recently from 906b9ee to 9f57d96 Compare December 8, 2022 00:42

arajasek marked this pull request as ready for review December 8, 2022 01:45

anorth reviewed Dec 11, 2022

View reviewed changes

arajasek force-pushed the asr/ipld-block-master branch from 9f57d96 to 30ac751 Compare December 12, 2022 18:41

jennijuju linked an issue Dec 13, 2022 that may be closed by this pull request

IPLD Block Abstraction #758

Closed

arajasek mentioned this pull request Dec 14, 2022

Introduce macros for actor dispatch code #947

Merged

arajasek marked this pull request as draft December 14, 2022 16:02

arajasek force-pushed the asr/ipld-block-master branch 5 times, most recently from ad37e93 to f85ce31 Compare December 15, 2022 02:17

arajasek commented Dec 15, 2022

View reviewed changes

arajasek marked this pull request as ready for review December 15, 2022 02:31

arajasek force-pushed the asr/ipld-block-master branch from b8bde98 to ca85aa8 Compare December 15, 2022 15:53

Stebalien reviewed Dec 15, 2022

View reviewed changes

arajasek force-pushed the asr/ipld-block-master branch from f8b12ef to bcd18ff Compare December 15, 2022 17:57

Stebalien approved these changes Dec 15, 2022

View reviewed changes

anorth approved these changes Dec 15, 2022

View reviewed changes

arajasek force-pushed the asr/ipld-block-master branch from bcd18ff to d5d2212 Compare December 15, 2022 22:33

ZenGround0 reviewed Dec 15, 2022

View reviewed changes

ZenGround0 reviewed Dec 16, 2022

View reviewed changes

ZenGround0 approved these changes Dec 16, 2022

View reviewed changes

arajasek added 7 commits December 16, 2022 16:24

Use Option<IpldBlock> for all message params

d39793e

Use actor_dispatch!

f2ea2e2

Fixup test_vm to use IpldBlock

3571d88

Clippy: cleanup

7cfe483

FVM::serialize_cbor now returns Option<IpldBlcok>

afa8dd1

Update frc42_dispatch, frc46_token, and fvm_actor_utils

70607f5

Address review

f00f278

arajasek force-pushed the asr/ipld-block-master branch from d5d2212 to 2c64ca7 Compare December 16, 2022 21:41

arajasek enabled auto-merge (squash) December 16, 2022 21:42

arajasek force-pushed the asr/ipld-block-master branch from 2c64ca7 to 1571382 Compare December 16, 2022 21:48

Address review

6451460

arajasek force-pushed the asr/ipld-block-master branch from 1571382 to 6451460 Compare December 16, 2022 22:00

arajasek merged commit 18f89be into master Dec 16, 2022

arajasek deleted the asr/ipld-block-master branch December 16, 2022 22:20

This was referenced Dec 19, 2022

Remove unused macro #969

Merged

Refactor: Use Option<IpldBlock> for method returns #971

Closed

anorth mentioned this pull request Jan 17, 2023

Defer loading and decoding the parameters block until it's actually used #1066

Open

jennijuju mentioned this pull request Jan 20, 2023

Recucsive evm call traces contain raw params filecoin-project/ref-fvm#1523

Closed

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

Use Option<IpldBlock> for all message params (filecoin-project#913)

442b3e7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Option<IpldBlock> for all message params #913

Use Option<IpldBlock> for all message params #913

arajasek commented Dec 5, 2022 •

edited

Loading

arajasek commented Dec 8, 2022

anorth left a comment

anorth Dec 11, 2022

arajasek Dec 15, 2022

anorth Dec 15, 2022

anorth commented Dec 11, 2022

codecov-commenter commented Dec 12, 2022 •

edited

Loading

arajasek commented Dec 14, 2022

arajasek Dec 15, 2022

arajasek commented Dec 15, 2022

anorth Dec 15, 2022

anorth Dec 15, 2022

anorth Dec 15, 2022

anorth Dec 15, 2022

ZenGround0 Dec 15, 2022

ZenGround0 Dec 15, 2022

arajasek Dec 16, 2022

ZenGround0 Dec 15, 2022

arajasek Dec 16, 2022

ZenGround0 Dec 15, 2022

ZenGround0 Dec 16, 2022

		@@ -371,11 +372,7 @@ impl<'bs> VM<'bs> {

		pub fn get_state<C: Cbor>(&self, addr: Address) -> Option<C> {

Use Option<IpldBlock> for all message params #913

Use Option<IpldBlock> for all message params #913

Conversation

arajasek commented Dec 5, 2022 • edited Loading

arajasek commented Dec 8, 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

anorth commented Dec 11, 2022

codecov-commenter commented Dec 12, 2022 • edited Loading

Codecov Report

arajasek commented Dec 14, 2022

Choose a reason for hiding this comment

arajasek commented Dec 15, 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

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 Dec 5, 2022 •

edited

Loading

codecov-commenter commented Dec 12, 2022 •

edited

Loading