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.

Sign up for GitHub

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

Jump to bottom

Updates to Actor events based on community feedback for NV22 #1531

Merged
merged 10 commits into from
Mar 21, 2024
Merged

Updates to Actor events based on community feedback for NV22 #1531

merged 10 commits into from
Mar 21, 2024

Conversation

aarshkshah1992
Copy link
Contributor

@aarshkshah1992 aarshkshah1992 commented Mar 18, 2024 •
edited
Loading

This PR updates Actor events to incorporate community feedback on the upcoming Actor events feature in NV22.
The corresponding FIP proposal update is at filecoin-project/FIPs#964.

TODO

  • Integration tests

@aarshkshah1992 aarshkshah1992 requested review from anorth and rvagg March 18, 2024 14:56
@aarshkshah1992 aarshkshah1992 changed the base branch from master to release/v13 March 18, 2024 14:56
anorth
anorth reviewed Mar 18, 2024
View reviewed changes
actors/miner/src/emit.rs Outdated Show resolved Hide resolved
actors/verifreg/src/emit.rs Outdated Show resolved Hide resolved
actors/verifreg/src/lib.rs Outdated
Comment on lines 147 to 150
let existing_cap =
st.get_verifier_cap(rt.store(), &verifier_addr)?.ok_or_else(|| {
actor_error!(illegal_argument, "verifier {} not found", verifier_addr)
})?;
Copy link
Member

Choose a reason for hiding this comment

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

I don't think adding a new state read is an acceptable cost to get this field in. I'm skeptical we should add "previous" values, but in this case if we decide that we do, please thread it out of the remove_verifier call instead (it's returned from the Map delete operation).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@anorth Have consensus with @rvagg that we don't need to add previous values anymore. We've changed the FIP update proposal accordingly.

actors/verifreg/src/lib.rs Outdated Show resolved Hide resolved
actors/verifreg/src/lib.rs Outdated Show resolved Hide resolved
@rvagg rvagg force-pushed the feat/actor-event-updates branch from 52f87e2 to f93c361 Compare March 19, 2024 05:42
@rvagg
Copy link
Member

rvagg commented Mar 19, 2024

(apologies for the bad push, I forgot to branch properly before committing a new thing, this is back to the original commit now)

@rvagg rvagg mentioned this pull request Mar 19, 2024
Closed
@rvagg rvagg force-pushed the feat/actor-event-updates branch from 58be210 to fb5a5d3 Compare March 20, 2024 05:37
anorth
anorth approved these changes Mar 20, 2024
View reviewed changes
actors/verifreg/src/emit.rs Outdated Show resolved Hide resolved
actors/verifreg/src/emit.rs Outdated Show resolved Hide resolved
actors/verifreg/src/lib.rs Outdated Show resolved Hide resolved
actors/verifreg/src/emit.rs Outdated Show resolved Hide resolved
actors/verifreg/tests/harness/mod.rs Outdated Show resolved Hide resolved
@rvagg
Copy link
Member

rvagg commented Mar 20, 2024

latest review feedback addressed, and I minimised the total diff a bit more so it should be slightly cleaner now. PTAL @anorth

anorth
anorth approved these changes Mar 20, 2024
View reviewed changes
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.

Thanks

@rvagg
Copy link
Member

rvagg commented Mar 21, 2024

Added term_start in the latest commit but it's not quite passing and needs a good eyeball. Currently extend_updated_sector_with_claims_test is failing, I'm asserting that term_start should be deal_start, but that's not what the event is coming out with. I think (if I'm reading expected vs actual right) that it's emitting 3708 but expecting 6588. I'm not sure how that would be the case.

@rvagg
Copy link
Member

rvagg commented Mar 21, 2024

Fixed that problem - I made the claim term_start to be the current epoch. The deal_start (StartEpoch on a deal proposal) was set to current epoch + 1 day. I believe makes sense because start epoch of a proposal is just a "no later than".

@aarshkshah1992
Copy link
Contributor Author

lgtm

@aarshkshah1992 aarshkshah1992 merged commit 2c73328 into release/v13 Mar 21, 2024
14 checks passed
@aarshkshah1992 aarshkshah1992 deleted the feat/actor-event-updates branch March 21, 2024 12:51
rvagg added a commit that referenced this pull request Jun 19, 2024
@aarshkshah1992 @rvagg
Updates to Actor events based on community feedback for NV22 (#1531)
9efe98c 
* 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 <[email protected]>
rvagg added a commit that referenced this pull request Jun 19, 2024
@aarshkshah1992 @rvagg
Updates to Actor events based on community feedback for NV22 (#1531)
0e0df51 
* 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 <[email protected]>
rvagg added a commit that referenced this pull request Jun 19, 2024
@rvagg
fixup! Updates to Actor events based on community feedback for NV22 (#…
79aa1ad 
…1531)
rvagg added a commit that referenced this pull request Jun 19, 2024
@aarshkshah1992 @rvagg
Updates to Actor events based on community feedback for NV22 (#1531)
7fe2a41 
* 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 <[email protected]>
rvagg added a commit that referenced this pull request Jun 19, 2024
@aarshkshah1992 @rvagg
Updates to Actor events based on community feedback for NV22 (#1531)
15f9695 
* 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 <[email protected]>
github-merge-queue bot pushed a commit that referenced this pull request Jun 21, 2024
@rvagg @aarshkshah1992
cherry-pick late release/v13 commits onto master (#1554)
1d8f082 
* 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 <[email protected]>

* 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 <[email protected]>

---------

Co-authored-by: Aarsh Shah <[email protected]>
ZenGround0 pushed a commit that referenced this pull request Jun 25, 2024
@rvagg @aarshkshah1992 @ZenGround0
cherry-pick late release/v13 commits onto master (#1554)
68895cb 
* 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 <[email protected]>

* 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 <[email protected]>

---------

Co-authored-by: Aarsh Shah <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anorth anorth anorth approved these changes

@rvagg rvagg rvagg approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@aarshkshah1992 @rvagg @anorth