Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Market Actor Events #1471

Merged
merged 8 commits into from
Nov 3, 2023

Conversation

aarshkshah1992
Copy link
Contributor

For #1464 .

This PR contains Market Actor events for the following deal lifecycle events:

  1. Deal published
  2. Deal activated
  3. Deal terminated (when a sector is terminated by the Miner -> all deals in the sector get terminated)
  4. Deal activated

We do not emit events for expired deals i.e. deals that have been published but not activated by their start epoch as that only ever happens in the cron job.

@aarshkshah1992 aarshkshah1992 changed the base branch from master to integration/actor-events October 30, 2023 11:32
@codecov-commenter
Copy link

codecov-commenter commented Oct 30, 2023

Codecov Report

❗ No coverage uploaded for pull request base (integration/actor-events@688d404). Click here to learn what that means.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@                     Coverage Diff                     @@
##             integration/actor-events    #1471   +/-   ##
===========================================================
  Coverage                            ?   91.09%           
===========================================================
  Files                               ?      151           
  Lines                               ?    28169           
  Branches                            ?        0           
===========================================================
  Hits                                ?    25661           
  Misses                              ?     2508           
  Partials                            ?        0           

.typ("deal-published")
.field_indexed("client", &client)
.field_indexed("provider", &provider)
.field_indexed("deal_id", &deal_id)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think just just "id" is sufficient as a concise key here, and in the events below. We used "id" for allocations and claims in verifreg.

&EventBuilder::new()
.typ("deal-published")
.field_indexed("client", &client)
.field_indexed("provider", &provider)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Stebalien for a second opinion, I suggested it might be worth including these fields since the deal ID is generated by the market actor, so a caller can't know what ID to filter for ahead of time. Many watchers will be interested in a small subset by party, and can avoid state inspection for most unwanted events with these fields.

However, by this logic we should also add client and provider to the verified allocation event too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Are we not including the deal label and/or CID? That seems like the more important identifier.
  2. Regardless, including the provider/client definitely make sense here.
  3. For the verified allocation event, I haven't thought enough about those to be sure? My guess is that the clients will want to track them (e.g., in a wallet), so including the client's ID is likely a good idea (at the very least).

Personally, I'd just include all fields we think might be relevant, then benchmark to make sure we haven't increased gas fees by too much.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See filecoin-project/FIPs#754 (reply in thread) for why we're not including all the fields that might be relevant. Extending that discussion is still welcome.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to @Stebalien's idea of atleast including the client and provider to the events as well and then revisiting them after gas costs benchmarking. The UX will be much better if clients can reduce state lookups for events that have nothing to do with what they're interested in.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I guess I'm not exactly being consistent here (I think I was mostly focused on the discussion around identifying the event type).

I think we do want to include the minimum amount of information necessary to:

  1. Identify the event (so we can query for more information).
  2. Determine if the event might be interesting (usually just the IDs of the parties involved). This may significantly decrease complexity/costs for developers.

At first blush, I wouldn't include things like amounts because they can be queried. However, I'm not entirely sure about that because queries:

  1. Can only only see the state between top-level messages.
  2. Can only efficiently see the state between tipsets.

E.g., if a token is transferred multiple times within a single top-level message, being able to query the state isn't sufficient.

So, really, there's a third item: Any information the user might not be able to accurately and/or efficiently query for.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's just do the parties involved for now. In the absence of events, tools like Sentinel have been able to calculate state changes by comparing before and after states.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tools like Sentinel have been able to calculate state changes by comparing before and after states.

I'd avoid relying on "sentinel managed to do it" as evidence of anything:

  1. Sentinel mostly cares about the state of the system, not the individual operations. People who work with tokens, etc., care about the individual transfers.
  2. Sentinel actually had undecidable edge-cases (e.g., why did a sector terminate).
  3. Sentinel relies on, e.g., the fact a user can't terminate and compact in a single top-level message. That kind of guarantee will eventually disappear as we allow more and more on-chain programmability.
  4. Building and maintaining Sentinel was quite a bit more painful than it would have been if we had had proper events.

That doesn't mean we should just throw everything we can in every event (we can always add more information later), just... avoid using sentinel as evidence.

Copy link
Member

@anorth anorth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good after renaming "deal_id" -> "id".

Please also update the verifreg events to include the parties' Actor ID, probably in a separate PR.

@aarshkshah1992
Copy link
Contributor Author

@anorth Have addressed the final round of review here. Please can you approve ?

Raised a separate PR to add parties information to verifreg events at #1477.

@aarshkshah1992 aarshkshah1992 merged commit 4b3eb90 into integration/actor-events Nov 3, 2023
15 checks passed
@aarshkshah1992 aarshkshah1992 deleted the feat/market-actor-events branch November 3, 2023 05:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants