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 sort stake pools by saturation #270

Merged

Conversation

lgobbi-atix
Copy link
Contributor

Context

The current sort order for results returned from the DbSyncStakePoolProvider.queryStakePools method is ordered by name. We require the interface to also include saturation.

Proposed Solution

  • Add saturation sort field with desc order as default
  • Adapt withSort function to be able to sort queries other than findPoolData
  • Refactor DbSyncStakePoolProvider.queryStakePools so it can identify which query to use to order the response

Important Changes Introduced

  • DbSyncStakePoolProvider.queryStakePools refactor
  • Added getQueryBySortType private method to DbSyncStakePoolProvider
  • Added sort by saturation to pool metrics query
  • Fix a bug where sorting by name was case sensitive, leading to results like: ['CLIO1','July 2021','banderini']
  • Fix a bug where sorting by name desc would return pools with no name first instead of last
  • Fixed tests related to those bugs

@lgobbi-atix lgobbi-atix force-pushed the feature/ADP-1793-Add-stake-pool-order-by-saturation branch from 7805dd8 to 9209e98 Compare June 6, 2022 13:27
@rhyslbw rhyslbw requested a review from ivaylo-andonov June 7, 2022 08:31
@lgobbi-atix lgobbi-atix force-pushed the feature/ADP-1793-Add-stake-pool-order-by-saturation branch from 9209e98 to 4fbaaac Compare June 7, 2022 14:57
Copy link
Member

@mkazlauskas mkazlauskas 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 overall. Query building is getting complicated (and therefore difficult to read), but I don't think there's an easy solution to that.

@ivaylo-andonov
Copy link
Contributor

ivaylo-andonov commented Jun 9, 2022

More of a question, considering the intense increase in complexity and theoretically more sorting fields requested in the future, do we think that maintaining modular DB queries + additional logic populating minimum requested data for a simple operation like sort is still the better option than having one composed DB query (with joined required tables) providing stake pool data at once (sorted, filtered and paginated), if it's feasible at all to happen at DB level? 🤔

@rhyslbw
Copy link
Member

rhyslbw commented Jun 9, 2022

@IvayloAndonov Please create a SPIKE for further exploration of this technical debt reducer in a later sprint.

Copy link
Member

@rhyslbw rhyslbw left a comment

Choose a reason for hiding this comment

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

I expect fix: make stake pool order by name case insensitive and move nulls to last to be a separate commit before either a single commit with the refactor + feature, or two extra commits.

@lgobbi-atix lgobbi-atix force-pushed the feature/ADP-1793-Add-stake-pool-order-by-saturation branch from 4fbaaac to ed0b249 Compare June 9, 2022 19:34
@lgobbi-atix
Copy link
Contributor Author

I expect fix: make stake pool order by name case insensitive and move nulls to last to be a separate commit before either a single commit with the refactor + feature, or two extra commits.

I left the first commit with the refactor + feature as it's kinda hard to separate them and added a new commit after with the fix

rhyslbw
rhyslbw previously approved these changes Jun 13, 2022
Copy link
Member

@rhyslbw rhyslbw 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 @lgobbi-atix.

mkazlauskas
mkazlauskas previously approved these changes Jun 13, 2022
Copy link
Member

@mkazlauskas mkazlauskas 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, nice work! 🎉

ivaylo-andonov
ivaylo-andonov previously approved these changes Jun 13, 2022
@rhyslbw rhyslbw requested review from ssong-iohk and iadmytro June 13, 2022 12:45
Copy link
Contributor

@ssong-iohk ssong-iohk left a comment

Choose a reason for hiding this comment

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

Can you add the test case (i.e. happy path - the request with filters/sort query parameters) to stakePoolHttpProvider.test.ts

@lgobbi-atix
Copy link
Contributor Author

lgobbi-atix commented Jun 15, 2022

Can you add the test case (i.e. happy path - the request with filters/sort query parameters) to stakePoolHttpProvider.test.ts

The provider interface didn't change. The response for those tests are mocked, it doesn't make sense to add the filters and everything there. Those cases should already be covered with https://github.com/input-output-hk/cardano-js-sdk/pull/270/files#diff-7bf95288ddba72cc435fbe56613884016d332bdba413afd23c5e1e7bcc69ce42R684

@lgobbi-atix lgobbi-atix requested a review from ssong-iohk June 15, 2022 13:32
@rhyslbw
Copy link
Member

rhyslbw commented Jun 16, 2022

it doesn't make sense to add the filters and everything there

Right, the HTTPService implementations are just plumbing, and we could even look at refactoring the base class to be usable without needing to extend.

Copy link
Contributor

@ssong-iohk ssong-iohk left a comment

Choose a reason for hiding this comment

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

LGTM.
I will run regression test against testnet then approve unless any issues are found.
And @lgobbi-atix I think you can resolve conflict.

refactor: improve stake pool queries to allow adding more sort fields
@lgobbi-atix lgobbi-atix dismissed stale reviews from ivaylo-andonov and mkazlauskas via 4ff843f June 16, 2022 13:04
@lgobbi-atix lgobbi-atix force-pushed the feature/ADP-1793-Add-stake-pool-order-by-saturation branch from ed0b249 to 4ff843f Compare June 16, 2022 13:04
Copy link
Contributor

@ssong-iohk ssong-iohk left a comment

Choose a reason for hiding this comment

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

Regression test will be conducted after merging ( due to tricky rebase). Any found issue at that time, will handle with a separate ticket(s). (Hopefully not)!

@ssong-iohk ssong-iohk merged commit 2a9abff into master Jun 16, 2022
@ssong-iohk ssong-iohk deleted the feature/ADP-1793-Add-stake-pool-order-by-saturation branch June 16, 2022 18:02
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