Skip to content

Commit

Permalink
Merge pull request #549 from onflow/fix-filter-endpoints-inconsistencies
Browse files Browse the repository at this point in the history 
Filter-related endpoints return `null` result instead of empty array
  • Loading branch information
@sideninja
sideninja authored Sep 16, 2024
2 parents 21d286e + a628542 commit 7b75617
Show file tree
Hide file tree
Showing 2 changed files with 40 additions and 65 deletions.
73 changes: 16 additions & 57 deletions api/pull.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ import (
"github.com/sethvargo/go-limiter"

"github.com/onflow/flow-evm-gateway/config"
"github.com/onflow/flow-evm-gateway/metrics"
errs "github.com/onflow/flow-evm-gateway/models/errors"
"github.com/onflow/flow-evm-gateway/services/logs"
"github.com/onflow/flow-evm-gateway/storage"
Expand Down Expand Up @@ -286,7 +285,7 @@ func (api *PullAPI) NewFilter(ctx context.Context, criteria filters.FilterCriter
}

// GetFilterLogs returns the logs for the filter with the given id.
// If the filter could not be found, `nil` is returned.
// If the filter could not be found or has expired, an error is returned.
func (api *PullAPI) GetFilterLogs(
ctx context.Context,
id rpc.ID,
Expand All @@ -300,56 +299,36 @@ func (api *PullAPI) GetFilterLogs(

filter, ok := api.filters[id]
if !ok {
return handleError[[]*gethTypes.Log](
fmt.Errorf("%w: filter by id %s does not exist", errs.ErrEntityNotFound, id),
api.logger,
metrics.NopCollector,
)
return nil, fmt.Errorf("filter by id %s does not exist", id)
}

if filter.expired() {
api.uninstallFilter(id)
return handleError[[]*gethTypes.Log](
fmt.Errorf("%w: filter by id %s has expired", errs.ErrEntityNotFound, id),
api.logger,
metrics.NopCollector,
)
return nil, fmt.Errorf("filter by id %s has expired", id)
}

logsFilter, ok := filter.(*logsFilter)
if !ok {
return handleError[[]*gethTypes.Log](
fmt.Errorf("%w: filter by id %s is not a logs filter", errs.ErrInvalid, id),
api.logger,
metrics.NopCollector,
return nil, fmt.Errorf(
"%w: filter by id %s is not a logs filter",
errs.ErrInvalid,
id,
)
}

current, err := api.blocks.LatestEVMHeight()
if err != nil {
return handleError[[]*gethTypes.Log](
err,
api.logger,
metrics.NopCollector,
)
return nil, err
}

result, err := api.getLogs(current, logsFilter)
if err != nil {
return handleError[[]*gethTypes.Log](
err,
api.logger,
metrics.NopCollector,
)
return nil, err
}

logs, ok := result.([]*gethTypes.Log)
if !ok {
return handleError[[]*gethTypes.Log](
fmt.Errorf("logs filter returned incorrect type: %T", result),
api.logger,
metrics.NopCollector,
)
return nil, fmt.Errorf("logs filter returned incorrect type: %T", result)
}

return logs, nil
Expand All @@ -362,37 +341,25 @@ func (api *PullAPI) GetFilterLogs(
// (pending)Log filters return []Log.
func (api *PullAPI) GetFilterChanges(ctx context.Context, id rpc.ID) (any, error) {
if err := rateLimit(ctx, api.ratelimiter, api.logger); err != nil {
return "", err
return nil, err
}

api.mux.Lock()
defer api.mux.Unlock()

f, ok := api.filters[id]
if !ok {
return handleError[any](
fmt.Errorf("%w: filter by id %s does not exist", errs.ErrEntityNotFound, id),
api.logger,
metrics.NopCollector,
)
return nil, fmt.Errorf("filter by id %s does not exist", id)
}

current, err := api.blocks.LatestEVMHeight()
if err != nil {
return handleError[any](
err,
api.logger,
metrics.NopCollector,
)
return nil, err
}

if f.expired() {
api.uninstallFilter(id)
return handleError[any](
fmt.Errorf("%w: filter by id %s has expired", errs.ErrEntityNotFound, id),
api.logger,
metrics.NopCollector,
)
return nil, fmt.Errorf("filter by id %s has expired", id)
}

var result any
Expand All @@ -404,19 +371,11 @@ func (api *PullAPI) GetFilterChanges(ctx context.Context, id rpc.ID) (any, error
case *logsFilter:
result, err = api.getLogs(current, filterType)
default:
return handleError[any](
fmt.Errorf("%w: non-supported filter type: %T", errs.ErrInvalid, filterType),
api.logger,
metrics.NopCollector,
)
return nil, fmt.Errorf("non-supported filter type: %T", filterType)
}

if err != nil {
return handleError[any](
err,
api.logger,
metrics.NopCollector,
)
return nil, err
}

f.updateUsed(current)
Expand Down
32 changes: 24 additions & 8 deletions tests/web3js/eth_filter_endpoints_test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,14 +39,18 @@ describe('eth_uninstallFilter', async () => {
})

describe('eth_getFilterLogs', async () => {
it('should return null for missing filter', async () => {
it('should return an error for missing filter', async () => {
let response = await helpers.callRPCMethod('eth_getFilterLogs', ['0xffa1'])

assert.equal(response.status, 200)
assert.isNull(response.body.result)
assert.isDefined(response.body.error)
assert.equal(
response.body.error.message,
'filter by id 0xffa1 does not exist'
)
})

it('should return null for expired filter', async () => {
it('should return an error for expired filter', async () => {
let response = await helpers.callRPCMethod(
'eth_newFilter',
[{ 'address': contractAddress }]
Expand All @@ -63,7 +67,11 @@ describe('eth_getFilterLogs', async () => {
response = await helpers.callRPCMethod('eth_getFilterLogs', [filterID])

assert.equal(response.status, 200)
assert.isNull(response.body.result)
assert.isDefined(response.body.error)
assert.equal(
response.body.error.message,
'filter by id ' + filterID + ' has expired'
)
})

it('should return error message for non-logs filter', async () => {
Expand Down Expand Up @@ -157,14 +165,18 @@ describe('eth_getFilterLogs', async () => {
})

describe('eth_getFilterChanges', async () => {
it('should return null for missing filter', async () => {
it('should return an error for missing filter', async () => {
let response = await helpers.callRPCMethod('eth_getFilterChanges', ['0xffa1'])

assert.equal(response.status, 200)
assert.isNull(response.body.result)
assert.isDefined(response.body.error)
assert.equal(
response.body.error.message,
'filter by id 0xffa1 does not exist'
)
})

it('should return null for expired filter', async () => {
it('should return an error for expired filter', async () => {
let response = await helpers.callRPCMethod(
'eth_newFilter',
[{ 'address': contractAddress }]
Expand All @@ -181,7 +193,11 @@ describe('eth_getFilterChanges', async () => {
response = await helpers.callRPCMethod('eth_getFilterChanges', [filterID])

assert.equal(response.status, 200)
assert.isNull(response.body.result)
assert.isDefined(response.body.error)
assert.equal(
response.body.error.message,
'filter by id ' + filterID + ' has expired'
)
})

it('should return empty array when there are no logs', async () => {
Expand Down

0 comments on commit 7b75617

Please sign in to comment.