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

feat: add query IDs filter #87

Merged
merged 12 commits into from
May 30, 2024
Merged

feat: add query IDs filter #87

merged 12 commits into from
May 30, 2024

Conversation

dcb9
Copy link
Contributor

@dcb9 dcb9 commented Mar 14, 2024

Currently relayer handles all queries in specified smart contracts, but that is not enough in some circumstances, such as in a big SaaS project, users may only want to relay for specific queries in the contracts. This feature complements to RELAYER_REGISTRY_ADDRESSES to further filter out messages being processed.
If the PR is approved I'll update the doc accordingly.

@pr0n00gler
Copy link
Collaborator

Thanks, we'll review it shortly!

@dcb9
Copy link
Contributor Author

dcb9 commented Mar 26, 2024

@sotnikov-s Thanks for your thorough reviews :). Changes have been made.

Copy link
Contributor

@sotnikov-s sotnikov-s left a comment

Choose a reason for hiding this comment

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

thank you very much! we'll test the feature and get back to you with more info

@dcb9 dcb9 requested a review from sotnikov-s March 27, 2024 07:24
@joldie777
Copy link
Contributor

The filter works great!

I used test_kv_query script to create several smart-contracts, each with a registered interchain query. After that, I've checked the filter with different combinations of the received smart-contract addresses and query IDs. And every time I got the expected result.

For example, let's say we have the following:

  • smart-contract with address="address1" which owns the registered query with ID="1"
  • smart-contract with address="address2" which owns the registered query with ID="2"

============================================================================================

For envs set as:

RELAYER_REGISTRY_ADDRESSES=address1,address2
RELAYER_REGISTRY_QUERY_IDS=1,2

or

RELAYER_REGISTRY_ADDRESSES=address1,address2
RELAYER_REGISTRY_QUERY_IDS=

or

RELAYER_REGISTRY_ADDRESSES=
RELAYER_REGISTRY_QUERY_IDS=1,2

ICQ processes queries with ID="1" and ID="2"

============================================================================================

For envs set as:

RELAYER_REGISTRY_ADDRESSES=address1
RELAYER_REGISTRY_QUERY_IDS=1,2

or

RELAYER_REGISTRY_ADDRESSES=address1,address2
RELAYER_REGISTRY_QUERY_IDS=1

ICQ processes only the query with ID="1"

============================================================================================

For envs set as:

RELAYER_REGISTRY_ADDRESSES=address2
RELAYER_REGISTRY_QUERY_IDS=1,2

or

RELAYER_REGISTRY_ADDRESSES=address1,address2
RELAYER_REGISTRY_QUERY_IDS=2

ICQ processes only the query with ID="2"

============================================================================================

@dcb9
Copy link
Contributor Author

dcb9 commented Mar 29, 2024

@joldie777 Thank you for your test and feeback! The procedure and the test cases are very concise and easy to reproduce. And we are delighted to contribute to Neutron eco!

@joldie777
Copy link
Contributor

@dcb9 Thank you for your contribution!

@pr0n00gler
Copy link
Collaborator

The PR looks great, thanks!

But could you add a simple unit test to cover new functionality?

If the PR is approved I'll update the doc accordingly.

Also please the documentation

@dcb9
Copy link
Contributor Author

dcb9 commented Apr 7, 2024

Hi @pr0n00gler here is the PR for doc neutron-org/neutron-docs#171
About unit test I'm not sure where should I put, do you mean neutron-integration-tests repo?

@pr0n00gler
Copy link
Collaborator

Hi @pr0n00gler here is the PR for doc neutron-org/neutron-docs#171

Thx, the doc PR is approved!

About unit test I'm not sure where should I put, do you mean neutron-integration-tests repo?

No, just a basic unit test that covers a functionality you introduced (that some queries are being filter out basically).
You can use this PR as references for the tests.

We are going to cover almost everything with unit tests and these PRs is a good start for that

@dcb9
Copy link
Contributor Author

dcb9 commented Apr 9, 2024

That's great, the PR set a good foundation, specifically it separated the implementations of RpcHttpClient and RestHttpQuery. This abstraction is required to mock test data for Subscriber.Subscribe, so should I use this PR as a base to develop the unit test(use git merge or rebase)?

@pr0n00gler
Copy link
Collaborator

That's great, the PR set a good foundation, specifically it separated the implementations of RpcHttpClient and RestHttpQuery. This abstraction is required to mock test data for Subscriber.Subscribe, so should I use this PR as a base to develop the unit test(use git merge or rebase)?

We've merged the PR, so you can just merge main into your branch and implement the test!

@dcb9
Copy link
Contributor Author

dcb9 commented Apr 16, 2024

Unit tests added, and it works perfectly!

Copy link
Contributor

@sotnikov-s sotnikov-s left a comment

Choose a reason for hiding this comment

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

sorry for such a delayed response and thank you very much for contribution!

@sotnikov-s sotnikov-s requested a review from pr0n00gler May 30, 2024 09:47
@sotnikov-s sotnikov-s requested a review from joldie777 May 30, 2024 09:47
@pr0n00gler pr0n00gler merged commit 95855d7 into neutron-org:main May 30, 2024
5 of 10 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.

5 participants