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

test: DDO onboarding non-market verified data #11603

Merged
merged 7 commits into from
Feb 7, 2024

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Jan 30, 2024

WIP. I'm planning on adding some events testing in this. This could also do with some refactoring, maybe moving some of this into the testkit because it's quite verbose but also some of it is repeated from other verifreg testing.

events := make([]types.ActorEvent, 0)
go func() {
for e := range evtChan {
fmt.Printf("%s Got ActorEvent: %+v", time.Now().Format(time.StampMilli), e)
Copy link
Member Author

Choose a reason for hiding this comment

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

currently doesn't show anything because they get filtered out due to address translation in the event filter

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed

require.NoError(t, err)

for _, evt := range evts {
fmt.Printf("Got ActorEvent: %+v", evt)
Copy link
Member Author

Choose a reason for hiding this comment

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

also doesn't show anything for the same reason

Copy link
Contributor

Choose a reason for hiding this comment

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

This is fixed.

for _, evt := range evts {
fmt.Printf("Got ActorEvent: %+v", evt)
}

Copy link
Member Author

Choose a reason for hiding this comment

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

from here below we use the existing lotus APIs to get receipts and their event logs for the whole chain, which works

params, err := actors.SerializeParams(&verifregtypes13.AddVerifiedClientParams{Address: verifiedClientAddr, Allowance: initialDatacap})
require.NoError(t, err)

msg := &types.Message{
Copy link
Contributor

Choose a reason for hiding this comment

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

we really need to extract some of this logic into a helper to be reused by multiple tests

fmt.Sprintf("height=%d", event.Height),
fmt.Sprintf("msg=%s", event.MsgCid),
fmt.Sprintf("emitter=%s", event.EmitterAddr),
fmt.Sprintf("reverted=%t", event.Reverted),
Copy link
Contributor

Choose a reason for hiding this comment

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

This information is not persisted on message receipts. How will client be informed about reverted events if they only depend on message receipts ?

@aarshkshah1992
Copy link
Contributor

Addes tests for the events API.

@aarshkshah1992 aarshkshah1992 marked this pull request as ready for review February 7, 2024 08:18
@aarshkshah1992 aarshkshah1992 requested a review from a team as a code owner February 7, 2024 08:18
@aarshkshah1992 aarshkshah1992 requested review from Stebalien and aarshkshah1992 and removed request for a team February 7, 2024 08:18
Copy link
Contributor

@aarshkshah1992 aarshkshah1992 left a comment

Choose a reason for hiding this comment

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

lgtm.

func matchEvents(t *testing.T, exp []types.ActorEvent, actual []types.ActorEvent) {
// height and tipset cid can mismatch because expected events are sourced using APIs that can put in different tipsets
for i := range exp {
exp[i].Height = 0
Copy link
Contributor

Choose a reason for hiding this comment

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

@rvagg The tipset/height we put in the buildActorEventsFromMessages function below on these events is always one ahead of the actual tipset/height these events are actually emitted at (as recorded by the CollectEvents function in lotus/chain/events/index.go).

I tried digging into it and it looks like this is because the node.StateSearchMsg function can return messages from a previous tipset as well. Let me know what you think here. I think this is a non-issue.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, that's very interesting. Probably a non issue as far as the tests are concerned but this is really helpful to know as I write docs.

@aarshkshah1992
Copy link
Contributor

itests will be green once the FFI/ref-fvm deps changes for NV22 are merged into this branch.

@aarshkshah1992
Copy link
Contributor

@rvagg Merging this so we can iron out the final parts in #11540.

@aarshkshah1992 aarshkshah1992 merged commit 4cfd4f0 into feat/built-in-actor-events-api Feb 7, 2024
89 of 90 checks passed
@aarshkshah1992 aarshkshah1992 deleted the rvagg/ddo-verified branch February 7, 2024 12:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ☑️ Done (Archive)
Development

Successfully merging this pull request may close these issues.

3 participants