Skip to content

Conversation

@gusinacio
Copy link
Contributor

@gusinacio gusinacio commented Apr 19, 2024

This PR aims to create the value check for indexer-service. This means that our system checks if the gateway is sending the value that we are requesting, otherwise it rejects the query.

Initially, I was planning to do a cache system to allow queries that contain the older cost model but it exploded really fast in complexity.

Given conversations with indexers, they seemed to not care to lose a few queries during 30 seconds until the gateway fetches again the cost model.

In case this becomes a problem to indexers, we can implement it again.

@github-actions
Copy link
Contributor

github-actions bot commented Apr 19, 2024

Pull Request Test Coverage Report for Build 11505248479

Details

  • 273 of 448 (60.94%) changed or added relevant lines in 15 files are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-1.3%) to 71.373%

Changes Missing Coverage Covered Lines Changed/Added Lines %
common/src/tap/checks/allocation_eligible.rs 0 1 0.0%
common/src/tap/checks/sender_balance_check.rs 0 1 0.0%
service/src/service.rs 0 1 0.0%
tap-agent/src/tap/context/checks/value.rs 0 1 0.0%
common/src/cost_model.rs 146 148 98.65%
common/src/tap.rs 0 2 0.0%
service/src/routes/cost.rs 0 2 0.0%
common/src/indexer_service/http/request_handler.rs 0 14 0.0%
common/src/tap/checks/value_check.rs 95 246 38.62%
Files with Coverage Reduction New Missed Lines %
common/src/indexer_service/http/request_handler.rs 2 0.0%
Totals Coverage Status
Change from base Build 11488787549: -1.3%
Covered Lines: 5101
Relevant Lines: 7147

💛 - Coveralls

@aasseman
Copy link
Contributor

Having the cache in the indexer-service raises a small risk though.
We treat indexer-service as a stateless service. So in the case an indexer-service was recently started, it wouldn't be aware of previous recent agora models, and that could lead to wrongly rejecting queries.
However the only other solution to this would be to have the indexer DB do the caching of models, which could be annoying to implement.

@aasseman aasseman linked an issue Apr 29, 2024 that may be closed by this pull request
@gusinacio gusinacio force-pushed the gusinacio/feat-minimum-value-check branch from 02dc7f2 to e139644 Compare September 19, 2024 16:55
@gusinacio gusinacio force-pushed the gusinacio/feat-minimum-value-check branch 6 times, most recently from b96d348 to cd976d4 Compare October 11, 2024 20:51
@gusinacio gusinacio force-pushed the gusinacio/feat-minimum-value-check branch 2 times, most recently from 1577edc to 3559623 Compare October 24, 2024 18:25
@gusinacio gusinacio marked this pull request as ready for review October 24, 2024 21:05
@suchapalaver suchapalaver self-requested a review October 25, 2024 16:28
suchapalaver
suchapalaver previously approved these changes Oct 25, 2024
Copy link
Collaborator

@suchapalaver suchapalaver left a comment

Choose a reason for hiding this comment

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

This looks good to me @gusinacio , a lot of work in the making!

I think the tests could be made more defensive and clearer about intention, but I understand some of the limitations we're facing with testing and I trust this has been tested extensively observationally.

@gusinacio gusinacio enabled auto-merge (squash) October 26, 2024 00:52
anirudh2
anirudh2 previously approved these changes Oct 28, 2024
Copy link
Member

@anirudh2 anirudh2 left a comment

Choose a reason for hiding this comment

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

Great work, @gusinacio! Lot's of great stuff in here. None of my requests are necessarily blocking, so I'm going to approve the PR. Your call whether to address them or not.

}
}

fn get_expected_value(&self, agora_query: &AgoraQuery) -> anyhow::Result<u128> {
Copy link
Member

Choose a reason for hiding this comment

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

The naming can be somewhat arbitrary. For example, here, you have a getter get_expected_value, but in cost_model.rs, you have getters named cost_models and cost_model.

Copy link
Member

Choose a reason for hiding this comment

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

Good with either, but just pick one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have cost_models and cost_model there because they map to GraphQL queries that indexer-service has.

I usually use get_ but it's a Java mind hahaha, maybe a future refactor I should remove all getters into more meaningful names.

Copy link
Member

Choose a reason for hiding this comment

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

I love that you thumbs-up'ed your own comment hahaha. Glad you agree with yourself 😂

@gusinacio gusinacio self-assigned this Oct 28, 2024
@gusinacio gusinacio dismissed stale reviews from anirudh2 and suchapalaver via 8442654 October 29, 2024 18:52
Signed-off-by: Gustavo Inacio <[email protected]>
Signed-off-by: Gustavo Inacio <[email protected]>
Signed-off-by: Gustavo Inacio <[email protected]>
Signed-off-by: Gustavo Inacio <[email protected]>
Signed-off-by: Gustavo Inacio <[email protected]>
Signed-off-by: Gustavo Inacio <[email protected]>
Signed-off-by: Gustavo Inacio <[email protected]>
Signed-off-by: Gustavo Inacio <[email protected]>
Signed-off-by: Gustavo Inacio <[email protected]>
Signed-off-by: Gustavo Inacio <[email protected]>
@gusinacio gusinacio force-pushed the gusinacio/feat-minimum-value-check branch from 8442654 to 8092cab Compare October 29, 2024 18:58
@gusinacio gusinacio requested a review from anirudh2 October 29, 2024 18:58
Copy link
Member

@anirudh2 anirudh2 left a comment

Choose a reason for hiding this comment

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

Thanks, @gusinacio!

@gusinacio gusinacio merged commit 1e4a3cd into main Oct 29, 2024
10 checks passed
@gusinacio gusinacio deleted the gusinacio/feat-minimum-value-check branch October 29, 2024 20:38
@github-actions github-actions bot mentioned this pull request Oct 29, 2024
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.

Implement receipt value check

4 participants