Use is_controlling_address in actors #948

sudo-shashank · 2022-12-14T15:27:14Z

Replace existing control address accessor with new is_controlling_address

Closes #816

anorth · 2022-12-14T21:03:34Z

I look forward to this, but please target master for this one.

arajasek · 2022-12-15T01:48:42Z

@anorth I think the right target branch is integration/builtin-api, since the APIs work isn't in master yet (and won't be until FIP-0050 is accepted)

anorth · 2022-12-15T22:31:14Z

Sorry, yes @arajasek is correct

sudo-shashank · 2022-12-16T10:28:18Z

@anorth some test cases are failing with failed to get a readonly copy of the state with the use of is_controlling_address
am i missing something here?

actors/market/Cargo.toml

actors/miner/src/lib.rs

actors/market/src/lib.rs

anorth

This looks like the right direction now. Please re-request review when the tests are passing.

anorth · 2023-01-18T21:59:55Z

actors/market/src/ext.rs

@@ -26,13 +26,27 @@ pub mod miner {
    use super::*;

    pub const CONTROL_ADDRESSES_METHOD: u64 = 2;


I hope you can remove this constant and the associated params object now it's unused.

@anorth didn't remove this because it's also used in escrow_address

builtin-actors/actors/market/src/lib.rs

Line 1354 in 3d501f9

let (owner_addr, worker_addr, _) = request_miner_control_addrs(rt, nominal)?;

we can rename this to request_miner_addrs if that is more suitable

anorth · 2023-01-18T22:02:37Z

We've now merged integration/builtin-api into master, so this can be rebased on master.

codecov-commenter · 2023-01-19T15:13:17Z

Codecov Report

Merging #948 (3d501f9) into master (e28bc90) will decrease coverage by 2.14%.
The diff coverage is 90.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #948      +/-   ##
==========================================
- Coverage   89.36%   87.22%   -2.14%     
==========================================
  Files          93       93              
  Lines       19590    19834     +244     
==========================================
- Hits        17507    17301     -206     
- Misses       2083     2533     +450

Impacted Files	Coverage Δ
actors/market/src/lib.rs	`63.66% <85.71%> (-27.36%)`	⬇️
actors/market/src/ext.rs	`90.00% <100.00%> (-10.00%)`	⬇️
test_vm/src/util.rs	`93.67% <100.00%> (-6.24%)`	⬇️
actors/miner/src/deadline_info.rs	`79.77% <0.00%> (-16.86%)`	⬇️
actors/verifreg/src/state.rs	`90.10% <0.00%> (-9.36%)`	⬇️
test_vm/src/lib.rs	`88.91% <0.00%> (-0.46%)`	⬇️
actors/power/src/lib.rs	`83.58% <0.00%> (+0.21%)`	⬆️
... and 1 more

sudo-shashank · 2023-01-20T15:54:37Z

@anorth this PR is ready for review

anorth

Thanks. Are there any other actors that use this (for a follow-up PR?)

anorth · 2023-01-23T18:58:57Z

actors/market/src/lib.rs

+            IpldBlock::serialize_cbor(&ext::miner::IsControllingAddressParam { address: caller })?,
+            TokenAmount::zero(),
+        )?)?;
+        let caller_ok = res.is_controlling;


Nit: inline the caller_ok variable.

anorth · 2023-01-23T19:00:08Z

actors/market/tests/harness.rs

@@ -166,6 +166,34 @@ pub fn expect_provider_control_address(
    expect_get_control_addresses(rt, provider, owner, worker, vec![])
 }

+pub fn expect_get_is_control_addresses(


This method has the wrong name. I think you should inline it into expect_provider_is_control_address, which is otherwise doing nothign.

sudo-shashank · 2023-01-24T09:41:45Z

Thanks. Are there any other actors that use this (for a follow-up PR?)

No i didn't find any other actor that uses this

sudo-shashank changed the base branch from next to integration/builtin-api December 15, 2022 16:11

sudo-shashank changed the base branch from integration/builtin-api to master December 15, 2022 16:11

sudo-shashank changed the base branch from master to next December 15, 2022 16:12

sudo-shashank force-pushed the shashank/control-address-#816 branch from 8eb879a to 57eec94 Compare December 16, 2022 08:04

sudo-shashank changed the base branch from next to integration/builtin-api December 16, 2022 08:04

sudo-shashank marked this pull request as ready for review January 3, 2023 21:16

sudo-shashank requested a review from anorth January 5, 2023 03:29

anorth reviewed Jan 8, 2023

View reviewed changes

actors/market/Cargo.toml Outdated Show resolved Hide resolved

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

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

arajasek force-pushed the integration/builtin-api branch 3 times, most recently from 6001a12 to 07b4f5d Compare January 12, 2023 15:56

sudo-shashank force-pushed the shashank/control-address-#816 branch 7 times, most recently from b8c70be to b6876d2 Compare January 18, 2023 11:37

anorth reviewed Jan 18, 2023

View reviewed changes

sudo-shashank force-pushed the shashank/control-address-#816 branch 2 times, most recently from 508b521 to 609845b Compare January 19, 2023 15:02

sudo-shashank changed the base branch from integration/builtin-api to master January 19, 2023 15:23

sudo-shashank changed the base branch from master to integration/builtin-api January 19, 2023 15:23

Use IsControllingAddress instead of request_miner_control_addrs

cef1560

sudo-shashank force-pushed the shashank/control-address-#816 branch from 609845b to 8794be0 Compare January 19, 2023 17:23

sudo-shashank changed the base branch from integration/builtin-api to master January 19, 2023 17:24

Fix market actor test

3d501f9

sudo-shashank force-pushed the shashank/control-address-#816 branch from 8794be0 to 3d501f9 Compare January 19, 2023 17:33

sudo-shashank requested a review from anorth January 19, 2023 18:08

anorth approved these changes Jan 23, 2023

View reviewed changes

sudo-shashank and others added 2 commits January 24, 2023 15:41

Cleanup

747cf86

Merge branch 'master' into shashank/control-address-#816

257c529

anorth enabled auto-merge (squash) January 24, 2023 19:07

anorth merged commit f1d0fb0 into master Jan 24, 2023

anorth deleted the shashank/control-address-#816 branch January 24, 2023 19:28

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

Use is_controlling_address in actors (filecoin-project#948)

9692ad2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use is_controlling_address in actors #948

Use is_controlling_address in actors #948

sudo-shashank commented Dec 14, 2022 •

edited

Loading

anorth commented Dec 14, 2022

arajasek commented Dec 15, 2022

anorth commented Dec 15, 2022

sudo-shashank commented Dec 16, 2022

anorth left a comment

anorth Jan 18, 2023

sudo-shashank Jan 19, 2023

sudo-shashank Jan 19, 2023

sudo-shashank Jan 19, 2023 •

edited

Loading

anorth Jan 24, 2023

anorth commented Jan 18, 2023

codecov-commenter commented Jan 19, 2023 •

edited

Loading

sudo-shashank commented Jan 20, 2023

anorth left a comment

anorth Jan 23, 2023

anorth Jan 23, 2023

sudo-shashank commented Jan 24, 2023

		@@ -26,13 +26,27 @@ pub mod miner {
		use super::*;

		pub const CONTROL_ADDRESSES_METHOD: u64 = 2;

Use is_controlling_address in actors #948

Use is_controlling_address in actors #948

Conversation

sudo-shashank commented Dec 14, 2022 • edited Loading

anorth commented Dec 14, 2022

arajasek commented Dec 15, 2022

anorth commented Dec 15, 2022

sudo-shashank commented Dec 16, 2022

anorth left a comment

Choose a reason for hiding this comment

anorth Jan 18, 2023

Choose a reason for hiding this comment

sudo-shashank Jan 19, 2023

Choose a reason for hiding this comment

sudo-shashank Jan 19, 2023

Choose a reason for hiding this comment

sudo-shashank Jan 19, 2023 • edited Loading

Choose a reason for hiding this comment

anorth Jan 24, 2023

Choose a reason for hiding this comment

anorth commented Jan 18, 2023

codecov-commenter commented Jan 19, 2023 • edited Loading

Codecov Report

sudo-shashank commented Jan 20, 2023

anorth left a comment

Choose a reason for hiding this comment

anorth Jan 23, 2023

Choose a reason for hiding this comment

anorth Jan 23, 2023

Choose a reason for hiding this comment

sudo-shashank commented Jan 24, 2023

sudo-shashank commented Dec 14, 2022 •

edited

Loading

sudo-shashank Jan 19, 2023 •

edited

Loading

codecov-commenter commented Jan 19, 2023 •

edited

Loading