-
Notifications
You must be signed in to change notification settings - Fork 59
Move listSortedRewardCoupons to common code DbTransferInputQueries #3646
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
Move listSortedRewardCoupons to common code DbTransferInputQueries #3646
Conversation
Signed-off-by: Divam <dfordivam@gmail.com>
- Add MintingDelegationCollectRewardsTrigger - Bump version of DbExternalPartyWalletStore - Bump version of DbUserWalletStore Squashed commits from the PR reviewed in #3470 Signed-off-by: Divam <dfordivam@gmail.com>
Squashed commits from PR reviewed in #3598 Signed-off-by: Divam <dfordivam@gmail.com>
Extracted duplicated listSortedRewardCoupons method from DbUserWalletStore and DbExternalPartyWalletStore into a new shared mixin trait. Signed-off-by: Divam <dfordivam@gmail.com>
meiersi-da
left a comment
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.
Thanks a lot. Looks good to me.
@OriolMunoz-da : could you do the final approval on this refactoring given that you are one of the core maintainers of this piece of code, and have more Scala experience than I have?
OriolMunoz-da
left a comment
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, very minor comments about the comments/docs
.../common/src/main/scala/org/lfdecentralizedtrust/splice/store/db/DbTransferInputQueries.scala
Outdated
Show resolved
Hide resolved
| issuingRoundsMap: Map[Round, IssuingMiningRound], | ||
| roundToIssuance: IssuingMiningRound => Option[BigDecimal], | ||
| limit: Limit, | ||
| ccValue: SQLActionBuilder = sql"rti.issuance", |
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.
is this value different anywhere? It's the same on this PR - fine to keep if it's a change necessary for a later PR
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.
It's same, and I am not sure if it would differ in future. Nevertheless I kept the code identical to the existing UserWallet store's to minimize diffs.
.../common/src/main/scala/org/lfdecentralizedtrust/splice/store/db/DbTransferInputQueries.scala
Outdated
Show resolved
Hide resolved
.../common/src/main/scala/org/lfdecentralizedtrust/splice/store/db/DbTransferInputQueries.scala
Outdated
Show resolved
Hide resolved
Extracted duplicated listSortedRewardCoupons method from DbUserWalletStore and DbExternalPartyWalletStore into a new shared mixin trait. PR reviewed in #3646 Signed-off-by: Divam <dfordivam@gmail.com>
98c805c to
f919249
Compare
|
Closing this as commits have been squashed and put on the feature branch |
Extracted duplicated listSortedRewardCoupons method from DbUserWalletStore and DbExternalPartyWalletStore into a new shared mixin trait. PR reviewed in #3646 Signed-off-by: Divam <dfordivam@gmail.com>
Fixes #3555
I implemented the deduplication slightly different than initially discussed.
Here I a new shared mixin trait with the DB sql API.
More APIs could be implemented in this for the
TransferInputStore, though I did not do that as part of this PR, to keep the changes minimal.Pull Request Checklist
Cluster Testing
/cluster_teston this PR to request it, and ping someone with access to the DA-internal system to approve it./hdm_teston this PR to request it, and ping someone with access to the DA-internal system to approve it.PR Guidelines
Fixes #n, and mention issues worked on using#nMerge Guidelines