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

[Access] Implement gRPC streaming endpoint SubscribeAccountStatuses #5406

Conversation

AndriiDiachuk
Copy link
Contributor

@AndriiDiachuk AndriiDiachuk commented Feb 16, 2024

In this PR was Introduced a new endpoint named SubscribeAccountStatuses within the state_stream Execution Data API to facilitate the streaming of two account fields: Address and Status at the gRPC level.

Related PR: onflow/flow#1433

@codecov-commenter
Copy link

codecov-commenter commented Feb 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 58.41%. Comparing base (5922cda) to head (897b34b).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5406      +/-   ##
==========================================
+ Coverage   55.65%   58.41%   +2.75%     
==========================================
  Files        1037      857     -180     
  Lines      101377    83094   -18283     
==========================================
- Hits        56424    48537    -7887     
+ Misses      40613    30941    -9672     
+ Partials     4340     3616     -724     
Flag Coverage Δ
unittests 58.41% <ø> (+2.75%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@AndriiDiachuk AndriiDiachuk marked this pull request as ready for review February 21, 2024 14:17
Copy link
Member

@durkmurder durkmurder left a comment

Choose a reason for hiding this comment

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

Nice, good job. Mostly stylistic comments.

@durkmurder durkmurder removed the request for review from yhassanzadeh13 February 27, 2024 10:27
Copy link
Contributor

@peterargue peterargue left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @AndriiDiachuk!

now that I'm looking at this more closely, I think this is actually just a more specialized version of filtering events by field.

For example, if we were filtering flow.AccountContractAdded events for a specific address, we would need to first find all events with that type, then capture any who's address field matches the provided address. It's not included in the initial scoping, but if we were to want to add a balance updated filter, we'd search for all FlowToken.TokenDeposited and FlowToken.TokenWithdrawn events, then return any matching the address.

I don't think we need to add full support for field based filtering for generic events, but this should be implemented in a way that makes that easier in the future.

In general, I think that looks like adding some additional functionality to the EventFilter, that supports filtering for a specific value within a specific named field.

maybe something like this

type EventFilter struct {
	hasFilters bool
	EventTypes map[flow.EventType]struct{}
	...
	EventFieldFilters map[flow.EventType][]FieldFilter
}

type FieldFilter struct {
	FieldName string
	TargetValue string
}

Then we could create EventFilters setup to match on the requested address for the account event types.

engine/access/state_stream/filter.go Outdated Show resolved Hide resolved
engine/access/state_stream/filter.go Outdated Show resolved Hide resolved
engine/access/state_stream/filter.go Outdated Show resolved Hide resolved
engine/access/state_stream/backend/handler.go Outdated Show resolved Hide resolved
engine/access/state_stream/backend/handler.go Show resolved Hide resolved
Copy link
Member

@durkmurder durkmurder left a comment

Choose a reason for hiding this comment

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

I have limited context on accounts but to me everything looks good. I have left some stylistic comments but implementation looks alright to me.

Copy link
Contributor

@peterargue peterargue left a comment

Choose a reason for hiding this comment

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

added a few more comments, but I think this should be the last batch

engine/access/state_stream/filter.go Outdated Show resolved Hide resolved
engine/access/state_stream/filter.go Outdated Show resolved Hide resolved
engine/access/state_stream/filter.go Outdated Show resolved Hide resolved
engine/access/state_stream/account_status_filter.go Outdated Show resolved Hide resolved
engine/access/state_stream/backend/handler.go Outdated Show resolved Hide resolved
engine/access/state_stream/backend/handler.go Outdated Show resolved Hide resolved
engine/access/state_stream/account_status_filter.go Outdated Show resolved Hide resolved
@AndriiDiachuk
Copy link
Contributor Author

@peterargue thanks for the comments. Updated protoc-gen-go-grpc as well in flow project. Ready for review.

…Diachuk/access-grpc-streaming-endpoint-SubscribeAccountStatuses
CoreEventAccountContractAdded,
CoreEventAccountContractUpdated,
CoreEventAccountContractRemoved:
addFilter(eventType, "address")
Copy link
Contributor

@peterargue peterargue Mar 29, 2024

Choose a reason for hiding this comment

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

since this is only run once, you can simplify this

switch eventType {
	...
	case CoreEventAccountContractRemoved:
		defaultCoreEventsMap[eventType] = map[string]struct{}{
			"address": {},
		}
}

Copy link
Contributor

@peterargue peterargue 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. great work!

@peterargue peterargue added this pull request to the merge queue Mar 29, 2024
Merged via the queue into onflow:master with commit c45d5b8 Mar 29, 2024
55 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants