Add client to verifier balance event #1533

aarshkshah1992 · 2024-03-20T08:46:18Z

This is based on FIP update proposal filecoin-project/FIPs#968

Adding the 'client' field to the verifier balance event makes it easy for users to track datacap allocations by verifiers to clients.

anorth · 2024-03-20T20:43:33Z

actors/verifreg/src/emit.rs

 ) -> Result<ActorEvent, ActorError> {
    EventBuilder::new()
        .typ("verifier-balance")
        .field_indexed("verifier", &verifier)
        .field("balance", &BigIntSer(balance))
+        .field_indexed("client", &client)


Given the list-of-pairs logical and physical representation of an event, we have two ways to represent an absent field:

include a k/v pair but the value is nil

don't include the key at all

I mostly would go for omitting the key rather than including it with a nil value. One argument against this is that it would make it harder to naively splat event k/v pairs into a static structure, e.g. with the handy schema deserialisation tools in some programming languages. However that kind of static schema doesn't work generally anyway - e.g. sector activation events have repeated keys for pieces in a sector. Some custom splatting library could of course be written to interpret repeated keys into lists, etc, but such software could also handle missing keys for options.

So.... omit the field instead?

anorth · 2024-03-20T20:47:22Z

actors/verifreg/tests/harness/mod.rs

-                .field("balance", &BigIntSer(&(verifier_balance - allowance)))
-                .build()?,
-        );
+        if client_resolved.id().is_ok() {


This should probably just unwrap. If the client can't be resolved, something is wrong with the test code and it should abort (e.g. the datacap transfer should also fail).

thread 'clients::rejects_unresolved_address' panicked at 'called Result::unwrap()on anErr value: NonIDAddress', actors/verifreg/tests/harness/mod.rs:242:64

That's why it's in here. But now I don't understand why it's OK to if the emit and not have an event in this case.

actors/verifreg/tests/harness/mod.rs

rvagg · 2024-03-20T22:34:07Z

rebased and force-pushed some changes in response to review (sorry, got changes and rebase mixed up so you own my changes now @aarshkshah1992!)

Stebalien · 2024-03-20T23:45:17Z

But note: i agree with #1533 (comment) and would consider simply omitting nil clients (unless we expect that this will be a useful filter)?

test failure

Stebalien · 2024-03-20T23:46:25Z

Hm. It looks like that test failure is real.

anorth · 2024-03-21T00:02:11Z

It is explicitly testing unresolved address behaviour. But I think the test harness is wrong at

    let client_resolved = rt.get_id_address(client).unwrap_or(*client);

It should just not expect any calls or events if the address is unresolvable.

rvagg · 2024-03-21T00:35:18Z

It should just not expect any calls or events if the address is unresolvable.

this is actually from a expect_abort so I think Aarsh was right here, it's expecting to get an abort from the call and it shouldn't emit the event. I'll put his if back and comment it.

rvagg · 2024-03-21T00:38:12Z

done

rvagg · 2024-03-21T01:03:36Z

Made "client" optional instead of nullable, so it won't appear if there is no client involved as per discussion on the FIP.

aarshkshah1992 · 2024-03-21T07:08:16Z

@rvagg Yes, we're expecting the call to abort as the test is testing the case where client can not be resolved.

aarshkshah1992 · 2024-03-21T07:08:37Z

lgtm

* update to Actor events * changes as per review * remvoe clippy * changes as per new FIP update * Update to latest PR, remove emit:: use from testing * all itests passing and green CI * address review feedback, minimise diff * feat: add term_start to claim events * fix: claim term_start at current epoch, not at deal_start * fix: s/deal_term/claim_term * add client to verifier balance event * fix: make "client" optional on verifier-balance event --------- Co-authored-by: Rod Vagg <rod@vagg.org>

* Updates to Actor events based on community feedback for NV22 (#1531) * update to Actor events * changes as per review * remvoe clippy * changes as per new FIP update * Update to latest PR, remove emit:: use from testing * all itests passing and green CI * address review feedback, minimise diff * feat: add term_start to claim events * fix: claim term_start at current epoch, not at deal_start * fix: s/deal_term/claim_term --------- Co-authored-by: Rod Vagg <rod@vagg.org> * Add client to verifier balance event (#1533) * update to Actor events * changes as per review * remvoe clippy * changes as per new FIP update * Update to latest PR, remove emit:: use from testing * all itests passing and green CI * address review feedback, minimise diff * feat: add term_start to claim events * fix: claim term_start at current epoch, not at deal_start * fix: s/deal_term/claim_term * add client to verifier balance event * fix: make "client" optional on verifier-balance event --------- Co-authored-by: Rod Vagg <rod@vagg.org> --------- Co-authored-by: Aarsh Shah <aarshkshah1992@gmail.com>

aarshkshah1992 and others added 6 commits March 18, 2024 18:52

update to Actor events

f93c361

changes as per review

ae9f6af

remvoe clippy

e79ac05

changes as per new FIP update

78a5719

Update to latest PR, remove emit:: use from testing

fb5a5d3

all itests passing and green CI

dc8d927

aarshkshah1992 requested review from rvagg and Stebalien March 20, 2024 08:49

anorth reviewed Mar 20, 2024

View reviewed changes

anorth mentioned this pull request Mar 20, 2024

FIP-0083: add "client" to verifier-balance event filecoin-project/FIPs#968

Merged

address review feedback, minimise diff

1174376

rvagg force-pushed the feat/verifier-balance-client branch from aa2745a to 0aee536 Compare March 20, 2024 22:29

rvagg approved these changes Mar 20, 2024

View reviewed changes

rvagg force-pushed the feat/verifier-balance-client branch 4 times, most recently from 9f768be to ea9fb24 Compare March 20, 2024 22:33

Stebalien previously approved these changes Mar 20, 2024

View reviewed changes

feat: add term_start to claim events

aadfcd3

rvagg force-pushed the feat/verifier-balance-client branch from ea9fb24 to 29d7d24 Compare March 21, 2024 00:38

rvagg force-pushed the feat/verifier-balance-client branch from 29d7d24 to 927f968 Compare March 21, 2024 01:03

fix: claim term_start at current epoch, not at deal_start

221d514

rvagg force-pushed the feat/verifier-balance-client branch from 927f968 to a3d0403 Compare March 21, 2024 02:43

anorth approved these changes Mar 21, 2024

View reviewed changes

rvagg and others added 3 commits March 21, 2024 20:34

fix: s/deal_term/claim_term

4343e28

add client to verifier balance event

7be3362

fix: make "client" optional on verifier-balance event

8f848a0

rvagg approved these changes Mar 21, 2024

View reviewed changes

rvagg force-pushed the feat/verifier-balance-client branch from a3d0403 to 8f848a0 Compare March 21, 2024 09:42

Base automatically changed from feat/actor-event-updates to release/v13 March 21, 2024 12:51

aarshkshah1992 merged commit 0f205c3 into release/v13 Mar 21, 2024
14 checks passed

aarshkshah1992 deleted the feat/verifier-balance-client branch March 21, 2024 12:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add client to verifier balance event #1533

Add client to verifier balance event #1533

aarshkshah1992 commented Mar 20, 2024

anorth Mar 20, 2024

anorth Mar 20, 2024

rvagg Mar 20, 2024

rvagg commented Mar 20, 2024

Stebalien commented Mar 20, 2024

Stebalien commented Mar 20, 2024

anorth commented Mar 21, 2024

rvagg commented Mar 21, 2024

rvagg commented Mar 21, 2024

rvagg commented Mar 21, 2024

aarshkshah1992 commented Mar 21, 2024

aarshkshah1992 commented Mar 21, 2024

Add client to verifier balance event #1533

Add client to verifier balance event #1533

Conversation

aarshkshah1992 commented Mar 20, 2024

anorth Mar 20, 2024

Choose a reason for hiding this comment

anorth Mar 20, 2024

Choose a reason for hiding this comment

rvagg Mar 20, 2024

Choose a reason for hiding this comment

rvagg commented Mar 20, 2024

Stebalien commented Mar 20, 2024

Stebalien commented Mar 20, 2024

anorth commented Mar 21, 2024

rvagg commented Mar 21, 2024

rvagg commented Mar 21, 2024

rvagg commented Mar 21, 2024

aarshkshah1992 commented Mar 21, 2024

aarshkshah1992 commented Mar 21, 2024