FIP-0049 (actor events): align with implementation. #581

raulk · 2022-12-27T18:58:15Z

Mostly aligns FIP with the final implementation, adds a table of contents, product considerations. Missing gas costs.

FIPS/fip-0049.md

jennijuju · 2022-12-28T02:12:52Z

FIPS/fip-0049.md

 ## Simple Summary

 This FIP adds the ability for actors to emit externally observable events during
 execution. Events are fire-and-forget, and can be thought of as a side effect
 from execution. The payloads of events are self-describing objects signalling
-that some important circumstance, action or transition has ocurred. Actor events
+that some relevant circumstance, action or transition has ocurred. Actor events


Suggested change

that some relevant circumstance, action or transition has ocurred. Actor events

that some relevant state transition has occurred. Actor events

Can you please explain your suggestion here, @jennijuju? (Generally, for all other comments, please explain your suggestion unless it's a typo).

We do not want to limit actor events to state transitions only. For example, a forbidden action could not result in a state transition, but still be recorded as an event (e.g. TokenTransferDenied).

AFAIK, (in etherum), any actions that cause a failed message execution will lead to all state changes to be reverted, including the events. (quick google verification, here)

We do not want to limit actor events to state transitions only. For example, a forbidden action could not result in a state transition,

In this example, according to the definition below: When an actor exits abnormally (exit code > 0), events emitted from that invocation and its transitive callee are discarded. Could you please provide an example that a forbidden action in actors that will result in an exit code =0 (successful execution?)

Still seeking for an explanation.

FIPS/fip-0049.md

jennijuju · 2022-12-28T02:27:05Z

FIPS/fip-0049.md


 ## Specification

 ### New chain types

-We introduce a DAG-CBOR encoded `Event` type to represent an actor event.
+We introduce a DAG-CBOR encoded `StampedEvent` type to represent an actor event
+as committed on chain.


Suggested change

as committed on chain.

as emitted on chain.

No, that's incorrect.

elaborate why please? what is an committed event and whats an emitted event?

FIPS/fip-0049.md

jennijuju · 2022-12-28T03:06:39Z

FIPS/fip-0049.md

@@ -236,37 +253,18 @@ dynamic costs:
 > TODO need to make syscall gas dynamic based on param length


❗ todo is needed before moving to last call

FIPS/fip-0049.md

jennijuju · 2022-12-28T03:11:11Z

FIPS/fip-0049.md

+[`filecoin-project/ref-fvm`], and its first adoption is within the EVM runtime
+actor under [`filecoin-project/builtin-actors`].
+
+### Client handling


can you link a impl for indexing?

It's not on master yet, so unsafe to link. I will add a PR link, but generally not happy doing that.

FIPS/fip-0049.md

mriise · 2022-12-30T03:36:28Z

FIPS/fip-0049.md

@@ -304,28 +336,75 @@ break some of their code.

 ## Test Cases

-TODO.
+Refer to events test cases in the [reference
+implementation](https://github.com/filecoin-project/ref-fvm/blob/master/testing/integration/tests/events_test.rs).


Suggested change

implementation](https://github.com/filecoin-project/ref-fvm/blob/master/testing/integration/tests/events_test.rs).

implementation](https://github.com/filecoin-project/ref-fvm/blob/master/testing/integration/tests/events_test.rs). [permalink](https://github.com/filecoin-project/ref-fvm/blob/d8f5d86f28ecd03a9934240134dae83a9754f72e/testing/integration/tests/events_test.rs)

raulk · 2022-12-30T17:41:00Z

FIPS/fip-0049.md

+Event {
+    emitter: 1234,
+    entries: [
+        (0x03, "t1", <32-byte array>),  // CBOR major type 2
+        (0x03, "t2", <32-byte array>),  // CBOR major type 2
+        (0x03, "t3", <32-byte array>),  // CBOR major type 2
+        (0x03, "t4", <32-byte array>),  // CBOR major type 2
+        (0x03, "p", <32-byte array>),   // CBOR major type 2
+    ],
+}


These keys are more compact since we'll be charging for bytes indexed, which includes keys.

jennijuju · 2022-12-30T20:52:21Z

@kaitlin-beegle - I'd like either @anorth or @arajasek to review this FIP before we move this DRAFT to LAST CALL, thanks!

FIPS/fip-0049.md

anorth

Looks good except:

Gas cost parameters are still TODO

FIPS/fip-0049.md

anorth · 2023-01-05T20:04:09Z

FIPS/fip-0049.md

-        (0x01, "receiver", <id address>),
-        (0x01, "token_id", <identifier>),
+    entries: [
+        (0x01, "transfer", null),


Nit: why is this inconsistent with the fungible token transfer using ("type", "transfer")?

I added this example with a different key-value style to demonstrate a usage of the "index by key" flag. Since user code will pay per byte, I expect devs will come up with ways to produce compact event representations without information loss. On an abstract level, this follows this pattern for a schema:

{ "transfer": { "sender": 1111, "receiver": 2222, "token_id": "0x12345678" } }

As opposed to:

{ "type": "transfer", "sender": 1111, "receiver": 2222, "token_id": "0x12345678" }

anorth · 2023-01-05T20:13:26Z

FIPS/fip-0049.md

+Note that because the FVM does not write receipts to the blockstore, neither
+does it write the events AMT themselves. Doing so would result in a dangling
+write. Clients are responsible for managing event data as they wish, just like
+they're responsible for managing receipts.


The correctness of this is dubious. The VM might not write, but the node probably does, or certainly could, in order to support subsequent queries. I think this paragraph could just be dropped without loss.

Which part is incorrect? The FVM certainly does not write receipts, so writing the event AMTs would lead to a dangling write since they would be unreferenced unless the client writes the receipt.

Basically what this is saying is: "look client, you're responsible for managing receipts, and that includes events."

FIPS/fip-0049.md

jennijuju · 2023-01-06T18:43:49Z

I lifted my change request, however, there are still resolved conversation on this PR that I think authors should consider before we move it to last call.

anorth · 2023-01-08T20:30:45Z

@raulk ping us when you've pushed any final edits and I can merge (overriding branch protection) or @jennijuju can approve (necessary to unblock merging).

FIP-0049 (actor events): round of editing. Missing gas costs.

2cec4c2

raulk requested review from momack2, arajasek, jennijuju, kaitlin-beegle and anorth as code owners December 27, 2022 18:58

jennijuju requested changes Dec 28, 2022

View reviewed changes

raulk added 10 commits December 28, 2022 14:26

fix nits.

cb88b82

add a link to DAG-CBOR spec.

a3a4467

add access control example.

06da825

elaborate on how events are committed on chain.

744fba1

address nits around flags.

2c4e9a6

update the syscall.

786201d

add test cases.

48e5e66

clarify 'external'.

742a46a

cover fatal errors.

9fb21c5

link to indexing implementation in Lotus.

f99cd75

mriise reviewed Dec 30, 2022

View reviewed changes

raulk added 5 commits December 30, 2022 11:34

paint the shed.

a9ba904

another editing pass.

d195975

restrict values to DAG-CBOR byte strings; add Eth event.

b4dcba9

do not insert the event AMT into the blockstore.

151d691

specify error situations in emit_event syscall.

c6302d3

raulk force-pushed the raulk/fip-0049-edit branch from bfba6a4 to c6302d3 Compare December 30, 2022 16:22

raulk added 5 commits December 30, 2022 16:30

add Chain commitment section; align with implementation.

218c219

restructure sections.

1b370b6

rearchitect gas costs.

0f5dee4

various edits.

d06fdf1

add security considerations.

c547b15

raulk commented Dec 30, 2022

View reviewed changes

maciejwitowski reviewed Jan 3, 2023

View reviewed changes

FIPS/fip-0049.md Outdated Show resolved Hide resolved

minor edit for clarity.

05f693f

anorth approved these changes Jan 5, 2023

View reviewed changes

jennijuju self-requested a review January 6, 2023 18:42

jennijuju approved these changes Jan 8, 2023

View reviewed changes

raulk added 3 commits January 9, 2023 11:36

edits for clarity.

d002339

discuss choice of receipt data structure.

53bf960

remove comment about events for built-in actors.

2733c85

raulk force-pushed the raulk/fip-0049-edit branch from 5e8b4bc to 2733c85 Compare January 9, 2023 12:35

raulk added 2 commits January 9, 2023 13:51

multiple edits.

9d88dcf

update ToC.

38e6aa4

raulk merged commit 7e7f64f into master Jan 9, 2023

raulk deleted the raulk/fip-0049-edit branch January 9, 2023 13:54

raulk mentioned this pull request Jan 13, 2023

NV18: Filecoin EVM runtime + Actor Events + EthAccount + EAM + f4 addressing filecoin-project/lotus#9998

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FIP-0049 (actor events): align with implementation. #581

FIP-0049 (actor events): align with implementation. #581

raulk commented Dec 27, 2022

jennijuju Dec 28, 2022

raulk Dec 28, 2022 •

edited

Loading

jennijuju Dec 30, 2022

jennijuju Jan 13, 2023

jennijuju Dec 28, 2022

raulk Dec 28, 2022

jennijuju Dec 30, 2022

jennijuju Dec 28, 2022

jennijuju Dec 28, 2022

raulk Dec 28, 2022

mriise Dec 30, 2022

raulk Dec 30, 2022

jennijuju commented Dec 30, 2022

anorth left a comment

anorth Jan 5, 2023

raulk Jan 9, 2023

anorth Jan 5, 2023

raulk Jan 9, 2023

raulk Jan 9, 2023

jennijuju commented Jan 6, 2023

anorth commented Jan 8, 2023

	that some relevant circumstance, action or transition has ocurred. Actor events
	that some relevant state transition has occurred. Actor events

		@@ -236,37 +253,18 @@ dynamic costs:
		> TODO need to make syscall gas dynamic based on param length

	implementation](https://github.com/filecoin-project/ref-fvm/blob/master/testing/integration/tests/events_test.rs).
	implementation](https://github.com/filecoin-project/ref-fvm/blob/master/testing/integration/tests/events_test.rs). [permalink](https://github.com/filecoin-project/ref-fvm/blob/d8f5d86f28ecd03a9934240134dae83a9754f72e/testing/integration/tests/events_test.rs)

FIP-0049 (actor events): align with implementation. #581

FIP-0049 (actor events): align with implementation. #581

Conversation

raulk commented Dec 27, 2022

Choose a reason for hiding this comment

raulk Dec 28, 2022 • edited Loading

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

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

Choose a reason for hiding this comment

jennijuju commented Dec 30, 2022

anorth 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

Choose a reason for hiding this comment

jennijuju commented Jan 6, 2023

anorth commented Jan 8, 2023

raulk Dec 28, 2022 •

edited

Loading