search

filecoin-project / builtin-actors

The Filecoin built-in actors
Other
78 stars 74 forks source link

Add client to verifier balance event #1533

Closed aarshkshah1992 closed 3 months ago

aarshkshah1992 commented 3 months ago

This is based on FIP update proposal https://github.com/filecoin-project/FIPs/pull/968

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

rvagg commented 3 months ago

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 commented 3 months ago

But note: i agree with https://github.com/filecoin-project/builtin-actors/pull/1533#discussion_r1532828697 and would consider simply omitting nil clients (unless we expect that this will be a useful filter)?

Stebalien commented 3 months ago

Hm. It looks like that test failure is real.

anorth commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

done

rvagg commented 3 months ago

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

aarshkshah1992 commented 3 months ago

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

aarshkshah1992 commented 3 months ago

lgtm