-
Notifications
You must be signed in to change notification settings - Fork 58
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
feat: add sort stake pools by saturation #270
Conversation
7805dd8
to
9209e98
Compare
9209e98
to
4fbaaac
Compare
There was a problem hiding this 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.
packages/cardano-services/src/StakePool/DbSyncStakePoolProvider/util.ts
Outdated
Show resolved
Hide resolved
packages/cardano-services/src/StakePool/DbSyncStakePoolProvider/DbSyncStakePool.ts
Outdated
Show resolved
Hide resolved
packages/cardano-services/src/StakePool/DbSyncStakePoolProvider/DbSyncStakePool.ts
Outdated
Show resolved
Hide resolved
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? 🤔 |
@IvayloAndonov Please create a SPIKE for further exploration of this technical debt reducer in a later sprint. |
There was a problem hiding this 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.
4fbaaac
to
ed0b249
Compare
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 |
There was a problem hiding this 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.
There was a problem hiding this 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! 🎉
There was a problem hiding this 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
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 |
Right, the |
There was a problem hiding this 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
4ff843f
ed0b249
to
4ff843f
Compare
There was a problem hiding this 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)!
Context
The current sort order for results returned from the
DbSyncStakePoolProvider.queryStakePools
method is ordered by name. We require the interface to also includesaturation
.Proposed Solution
saturation
sort field withdesc
order as defaultwithSort
function to be able to sort queries other thanfindPoolData
DbSyncStakePoolProvider.queryStakePools
so it can identify which query to use to order the responseImportant Changes Introduced
DbSyncStakePoolProvider.queryStakePools
refactorgetQueryBySortType
private method toDbSyncStakePoolProvider
['CLIO1','July 2021','banderini']
desc
would return pools with no name first instead of last