Integrates datacap token with verified registry actor #514

anorth · 2022-08-08T04:10:31Z

Replaces the verified registry actor's client allowance state with calls to a built-in datacap token actor.

Note there's little type-safety around the token precision units vs the datacap units, but this will be resolved when we get a new release of the FVM SDK, which will come with a TokenAmount newtype that's not easily convertible to BigInt.

Before merging:

Add datacap token state inspection in integration tests

Closes #523
Closes #536

ZenGround0

First commit looks good.

ZenGround0 · 2022-08-11T15:16:19Z

actors/verifreg/src/lib.rs

-        let st = State::new(rt.store(), id_addr).map_err(|e| {
-            e.downcast_default(ExitCode::USR_ILLEGAL_STATE, "Failed to create verifreg state")
-        })?;
+        let st = State::new(rt.store(), id_addr, *DATACAP_TOKEN_ACTOR_ADDR)


Why store and use this instead of hardcoding dependence on this other builtin actor addr in the code? The later pattern is already everywhere.

I could go either way. I think this use of state is a pattern that would work better generally, outside built-ins. It also offers simple possibilities for migration to a new actor in the future.

That is a neat point, code cid could stay the same assuming state migration is possible

actors/verifreg/src/lib.rs

actors/verifreg/src/ext.rs

ZenGround0

Looks good so far

actors/verifreg/src/lib.rs

ZenGround0 · 2022-08-28T23:38:03Z

actors/verifreg/src/lib.rs

+        // Burn the client's data cap tokens.
+        let balance = balance_of(rt, &token, &client).context("failed to fetch balance")?;
+        let burnt = std::cmp::min(balance, params.data_cap_amount_to_remove);
+        destroy(rt, &token, &client, &burnt)


Inside of destroy we should have a check against a balance of 0 to optimize away an unnecessary send.

I don't think it's worth the complexity. This is a hard-to-invoke method that's explicitly oriented toward invoking this destroy function exactly once.

ZenGround0 · 2022-08-28T23:51:18Z

actors/verifreg/src/lib.rs

+    BS: Blockstore,
+    RT: Runtime<BS>,
+{
+    let token_amt = datacap_to_tokens(amount);


I get that datacap (and storage power etc) are conceptual tokens. But I'm worried about using the TokenAmount type to denote amounts of other token types since this is walking back the norm of calling different token types by different type aliases especially since the rest of the builtin actors are written assuming TokenAmount == FILAmount

Instead of importing ref-fvm the fungible token library could implement a FungibleTokenAmount trait that DataCap and others could implement.

The DataCap type or conceptual data cap? DataCap values here are not interchangeable with token amounts and are not conceptually tokens. The DataCap type here is not a token. I will happily delete the DataCap type since it offers nothing except documentation of the intended role of a BigInt.

the norm of calling different token types by different type aliases

Maybe this is just some imprecise definitions, but I don't think there is any such norm since prior to this branch there are no other token types. There are just other BigInts. The TokenAmount shall not be a mere alias.

The token standard proposal explicitly equates the type of user-programmed tokens with the native Filecoin built-in token. I haven't got the new TokenAmount type from the FVM yet, but once that comes this will all be type-safe. The token library will use the same TokenAmount. DataCap will not be convertible.

actors/verifreg/src/state.rs

ZenGround0 · 2022-08-29T00:01:05Z

actors/verifreg/src/testing.rs

-        Err(e) => acc.add(format!("error loading clients {e}")),
-    }
-
-    // check verifiers and clients are disjoint


TODO: I think we should still check this since we appear to be trying to maintain the invariant. It will become a cross DataCap Verifreg check

Yes, #537 is for that.

Kubuxu · 2022-08-29T23:06:50Z

One thing I noticed, that might be an issue is that the new .context and .context_with_code will cause allocations and formatting to be called even in the success case (as far as I know). This is the reason why for example Option has ok_or and ok_or_else exist, the latter takes a closure so the "expensive" operation doesn't need to be performed when it isn't needed.

Kubuxu

Apart from the context+format! issue which IMO is significant but can be addressed separately. LGTM.

anorth force-pushed the anorth/verifreg-tokenintegration branch from ce16c81 to 76478a7 Compare August 9, 2022 02:16

anorth force-pushed the anorth/datacap-token branch from 3c4f9d6 to 3582f12 Compare August 9, 2022 02:18

anorth force-pushed the anorth/verifreg-tokenintegration branch from 76478a7 to ea3444a Compare August 9, 2022 02:55

anorth force-pushed the anorth/datacap-token branch 5 times, most recently from 1014d9d to e61e26e Compare August 11, 2022 00:58

Base automatically changed from anorth/datacap-token to decouple-fil+ August 11, 2022 01:26

anorth force-pushed the anorth/verifreg-tokenintegration branch from ea3444a to e079141 Compare August 11, 2022 08:06

anorth marked this pull request as ready for review August 11, 2022 08:07

anorth requested a review from ZenGround0 August 11, 2022 08:07

anorth mentioned this pull request Aug 11, 2022

Change TokenAmount type to a new-type filecoin-project/ref-fvm#704

Closed

ZenGround0 reviewed Aug 11, 2022

View reviewed changes

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

ZenGround0 reviewed Aug 11, 2022

View reviewed changes

actors/verifreg/src/ext.rs Show resolved Hide resolved

anorth force-pushed the decouple-fil+ branch from 17639ac to 4f5775a Compare August 16, 2022 04:48

anorth force-pushed the anorth/verifreg-tokenintegration branch from e079141 to a94d10b Compare August 16, 2022 04:49

anorth marked this pull request as draft August 16, 2022 23:31

anorth force-pushed the anorth/verifreg-tokenintegration branch 3 times, most recently from 92a6dbb to 5c58a71 Compare August 23, 2022 01:43

anorth force-pushed the decouple-fil+ branch 3 times, most recently from fef6314 to e9f4498 Compare August 28, 2022 23:23

anorth added 3 commits August 29, 2022 11:34

Replace verified clients state with balances of data cap token.

574cb7a

Use datacap token whole units, update integration tests

4ec3e59

Hook up state invariant check

f71b0e7

anorth force-pushed the anorth/verifreg-tokenintegration branch from 5c58a71 to f71b0e7 Compare August 28, 2022 23:38

ZenGround0 reviewed Aug 29, 2022

View reviewed changes

Fix tests

5a7b7d0

anorth marked this pull request as ready for review August 29, 2022 01:37

Review and clippy

d518418

anorth force-pushed the anorth/verifreg-tokenintegration branch from 61f4d7f to d518418 Compare August 29, 2022 02:44

anorth requested a review from Kubuxu August 29, 2022 03:35

Kubuxu approved these changes Aug 29, 2022

View reviewed changes

anorth merged commit 4033db9 into decouple-fil+ Aug 30, 2022

anorth deleted the anorth/verifreg-tokenintegration branch August 30, 2022 01:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrates datacap token with verified registry actor #514

Integrates datacap token with verified registry actor #514

anorth commented Aug 8, 2022 •

edited

Loading

ZenGround0 left a comment

ZenGround0 Aug 11, 2022

anorth Aug 16, 2022

ZenGround0 Aug 28, 2022

ZenGround0 left a comment

ZenGround0 Aug 28, 2022

anorth Aug 29, 2022

ZenGround0 Aug 28, 2022

ZenGround0 Aug 28, 2022

anorth Aug 29, 2022 •

edited

Loading

ZenGround0 Aug 29, 2022

anorth Aug 29, 2022

Kubuxu commented Aug 29, 2022 •

edited

Loading

Kubuxu left a comment •

edited

Loading

Integrates datacap token with verified registry actor #514

Integrates datacap token with verified registry actor #514

Conversation

anorth commented Aug 8, 2022 • edited Loading

ZenGround0 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

ZenGround0 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

anorth Aug 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kubuxu commented Aug 29, 2022 • edited Loading

Kubuxu left a comment • edited Loading

Choose a reason for hiding this comment

anorth commented Aug 8, 2022 •

edited

Loading

anorth Aug 29, 2022 •

edited

Loading

Kubuxu commented Aug 29, 2022 •

edited

Loading

Kubuxu left a comment •

edited

Loading