events: Remove unsupported filters #19617
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Skipped Deployments
|
| @@ -1012,8 +1012,9 @@ impl IndexerReader { | |||
| .await; | |||
| } else { | |||
| let main_where_clause = match filter { | |||
| EventFilter::Package(package_id) => { | |||
| format!("package = '\\x{}'::bytea", package_id.to_hex()) | |||
| EventFilter::All(fs) if fs.is_empty() => { | |||
There was a problem hiding this comment.
(Adding support for "All": [] to the indexer reader -- this was a rare case where IndexerReader didn't have a way to do something that the fullnode did).
| impl EventFilter { | ||
| fn try_matches(&self, item: &SuiEvent) -> SuiResult<bool> { | ||
| Ok(match self { | ||
| impl Filter<SuiEvent> for EventFilter { |
There was a problem hiding this comment.
This PR is removing Any, Not, Package, EventField which were not supported by either Fullnode or PG JSON-RPC, but they are part of the (deprecated) events subscription system on fullnodes. What is our policy on features like that?
There was a problem hiding this comment.
We are already well past the release where we said we'd initially remove support for subscriptions, they are effectively deprecated so we can probably choose to either remove these compound filters from the code and we can highlight that the feature is deprecated and support will continue to be pruned down till its removed, or we just leave it as is. In either case I think we should make sure that these filters are completely removed from documentation, i've found that they've been a constant source of confusion in the docs since they generally aren't supported
amnn
force-pushed
the
amnn/idx-reader-rm-to-addr
branch
from
d3fff82
to
03eb06d
Compare
amnn
force-pushed
the
amnn/idx-rm-complex-ev-filters
branch
from
0a4c5b3
to
4903b1c
Compare
| impl EventFilter { | ||
| fn try_matches(&self, item: &SuiEvent) -> SuiResult<bool> { | ||
| Ok(match self { | ||
| impl Filter<SuiEvent> for EventFilter { |
There was a problem hiding this comment.
We are already well past the release where we said we'd initially remove support for subscriptions, they are effectively deprecated so we can probably choose to either remove these compound filters from the code and we can highlight that the feature is deprecated and support will continue to be pruned down till its removed, or we just leave it as is. In either case I think we should make sure that these filters are completely removed from documentation, i've found that they've been a constant source of confusion in the docs since they generally aren't supported
| @@ -203,15 +203,16 @@ fn try_into_byte(v: &Value) -> Option<u8> { | |||
| #[serde_as] | |||
| #[derive(Clone, Debug, Serialize, Deserialize, JsonSchema)] | |||
| pub enum EventFilter { | |||
| /// Query by events that match all the given filters, or return all events if no filters are | |||
| /// supplied. | |||
| All(Vec<EventFilter>), | |||
There was a problem hiding this comment.
only empty vec is implemented, we can just do All here? one step further, having an All filter seems un-necessary and we can change query: EventFilter to Option<EventFilter>? Both can be more breaking so okay to me if we want to minimize the breaking here.
There was a problem hiding this comment.
Yeah, I'm not too happy with how All works today and the fact that this is how you communicate "no filter" (vs. how it's done for transactions which is to omit the filter), but I opted to do it this way to minimise breakage.
There was a problem hiding this comment.
would it be clearer to have this be a fixed-size, 0-sized array? Assuming that it minimizes breakage in the same way as we have here?
There was a problem hiding this comment.
I initially tried to make this a unit type for similar reasons, but that made serde very unhappy. I can try [EventFilter; 0] though!
amnn
force-pushed
the
amnn/idx-reader-rm-to-addr
branch
from
03eb06d
to
e801ee7
Compare
amnn
force-pushed
the
amnn/idx-rm-complex-ev-filters
branch
from
4903b1c
to
3524514
Compare
## Description
This PR removes EventFilters from JSON-RPC that aren't supported
already by fullnode-backed JSON-RPC:
- Combination filters (`All`, `Any`, `Not`, `And`, `Or`)
- `Package`
- `EventField`
These filters were, however, supported by the now-deprecated
subscription system, so a question remains whether they are safe to
remove.
## Test plan
Manually tested, in particular `All: []` support, which we do still need
to keep as a way to get all filters (this was also added to the
`IndexerReader` implementation):
```
sui$ cargo run --bin sui -- --force-regenesis \
--with-indexer --with-faucet
```
...run for some time, so that some system events accumulate, then test
that we get them from both the fullnode-backed JSONRPC and
indexer-backed:
Fullnode:
```
curl -LX POST "http://localhost:9000" \
--header 'Content-Type: application/json' \
--data '{
"jsonrpc": "2.0",
"id": 1,
"method": "suix_queryEvents",
"params": [
{
"All": []
},
null,
50,
true
]
}' | jq .
```
Indexer:
```
curl -LX POST "http://localhost:9124" \
--header 'Content-Type: application/json' \
--data '{
"jsonrpc": "2.0",
"id": 1,
"method": "suix_queryEvents",
"params": [
{
"All": []
},
null,
50,
true
]
}' | jq .
```
amnn
force-pushed
the
amnn/idx-rm-complex-ev-filters
branch
from
3524514
to
800fe53
Compare
…19618) ## Description Broaden the definition of "affected object" to include objects that a transaction creates and then immediately wraps, or objects that it unwraps and then immediately deletes. This ensures that a transaction will show up in the response to an `affectedObject` query if and only if the Object's ID shows up in its `objectChanges` response. ## Test plan Updated GraphQL E2E test: ``` sui$ cargo nextest run -p sui-graphql-e2e-tests \ --features staging -- affected_object ``` ## Stack - #19474 - #19614 - #19615 - #19616 - #19617 --- ## Release notes Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required. For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates. - [ ] Protocol: - [ ] Nodes (Validators and Full nodes): - [x] Indexer: A transaction's "affected objects" include objects that it created and wrapped, or unwrapped. - [x] JSON-RPC: A transaction's "affected objects" include objects that it created and wrapped, or unwrapped. - [x] GraphQL: A transaction's "affected objects" include objects that it created and wrapped, or unwrapped. - [ ] CLI: - [ ] Rust SDK: - [ ] REST API:
## Description Temporarily re-enable `EventFilter::Any` as it is still in use in some places. This is a partial revert of #19617. ## Test plan CI
## Description Temporarily re-enable `EventFilter::Any` as it is still in use in some places. This is a partial revert of #19617. ## Test plan CI --- ## Release notes Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required. For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates. - [ ] Protocol: - [x] Nodes (Validators and Full nodes): Temporarily re-enable `EventFilter::Any` as a kind of event subscription filter. Note that subscriptions are deprecated. This means they are not officially supported, nor actively maintained. - [ ] Indexer: - [ ] JSON-RPC: - [ ] GraphQL: - [ ] CLI: - [ ] Rust SDK: - [ ] REST API:
## Description Temporarily re-enable `EventFilter::Any` as it is still in use in some places. This is a partial revert of #19617. ## Test plan CI --- ## Release notes Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required. For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates. - [ ] Protocol: - [x] Nodes (Validators and Full nodes): Temporarily re-enable `EventFilter::Any` as a kind of event subscription filter. Note that subscriptions are deprecated. This means they are not officially supported, nor actively maintained. - [ ] Indexer: - [ ] JSON-RPC: - [ ] GraphQL: - [ ] CLI: - [ ] Rust SDK: - [ ] REST API:
## Description Temporarily re-enable `EventFilter::Any` as it is still in use in some places. This is a partial revert of #19617. ## Test plan CI --- ## Release notes Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required. For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates. - [ ] Protocol: - [x] Nodes (Validators and Full nodes): Temporarily re-enable `EventFilter::Any` as a kind of event subscription filter. Note that subscriptions are deprecated. This means they are not officially supported, nor actively maintained. - [ ] Indexer: - [ ] JSON-RPC: - [ ] GraphQL: - [ ] CLI: - [ ] Rust SDK: - [ ] REST API:
Description
This PR removes EventFilters from JSON-RPC that aren't supported already by fullnode-backed JSON-RPC:
All,Any,Not,And,Or)PackageEventFieldThese filters were, however, supported by the now-deprecated subscription system, so a question remains whether they are safe to remove.
Test plan
Manually tested, in particular
All: []support, which we do still need to keep as a way to get all filters (this was also added to theIndexerReaderimplementation):...run for some time, so that some system events accumulate, then test that we get them from both the fullnode-backed JSONRPC and indexer-backed:
Fullnode:
Indexer:
Stack
Release notes
Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.
For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates.