DO NOT MERGE: Daml code for implementing traffic-based app rewards#3964
DO NOT MERGE: Daml code for implementing traffic-based app rewards#3964meiersi-da wants to merge 5 commits into
Conversation
80c89e5 to
824bd4d
Compare
meiersi-da
left a comment
There was a problem hiding this comment.
@moritzkiefer-da : I have a sketch of what I believe are all the choices and templates to make the switch to traffic-based app rewards possible at the Daml level. Some simple work is delayed via FIXMEs in the code.
Could I ask you for a review to check alignment, in particular:
- highlight implementation choices that you disagree with / consider fishy
- highlight aspects / workflows that are missing or that you are not sure how they'll work
You don't have to work too hard on the latter part. I expect that we also find them as part of writing up and reviewing the full design.
f2474d0 to
057486f
Compare
f3677b9 to
f126f7b
Compare
9566611 to
edc394a
Compare
will get to it on Monday |
moritzkiefer-da
left a comment
There was a problem hiding this comment.
Great work thanks!
e43454f to
afe0825
Compare
| extraArgs : ExtraArgs | ||
| -- ^ Extra arguments for extensibility. Set to empty, unless needed for specific implementations. | ||
| observer map (.beneficiary) newBeneficiaries | ||
| controller (view this).beneficiary |
There was a problem hiding this comment.
This makes RewardCoupons transferrable. It looks like a classic bearer instrument redeemable with the dso. I worry that this could be an issue for some stakeholders.
It will be added in a separate PR Signed-off-by: Simon Meier <simon@digitalasset.com>
Signed-off-by: Simon Meier <simon@digitalasset.com>
Signed-off-by: Simon Meier <simon@digitalasset.com>
| round : Round | ||
| amuletPrice : Decimal | ||
| issuanceConfig : IssuanceConfig | ||
| tickDuration : RelTime |
There was a problem hiding this comment.
@meiersi-da I believe we want to pass the trafficPrice and rewardConfig to SummarizingMiningRound? I guess that was the intention of adding it to OpenMiningRound; to have this data available in SummarizingMiningRoundTrigger?
There was a problem hiding this comment.
What would you expect that trigger to do with this information? Right now the code is organized such that the Calculate/ProcessRewardsV2 workflow runs to the side of the SummarizingRound workflow.
There was a problem hiding this comment.
In SummarizingMiningRoundTrigger I need to determine the rewardConfig.mintingVersion for the round, and the CC totals for featuredAppRewardCoupons based on total traffic for round. I guess I can query the corresponding OpenMiningRound to fetch both of this info. But since the OpenMiningRound has been archived, it wont be in ACS, so fetch from update_history will be required.
There was a problem hiding this comment.
Ah I see. I now realize my mistake. I was thinking of the IssuingMiningRound. Your request makes sense. Please add the fields to copy the info from the OpenMiningRound.
There was a problem hiding this comment.
Another related concern is that the test code is still based on AppRewardCoupon count. Specifically
runNextIssuance
token-standard/splice-token-standard-test/daml/Splice/Testing/Registries/AmuletRegistry.daml
runNextIssuanceD
daml/splice-dso-governance-test/daml/Splice/Scripts/DsoTestUtils.daml
And we even use these APIs as it is in Splice.Scripts.TestRewardAccountingV2.
Do you think we should do any modifications to these?
The scala is simply injecting the totalFeaturedAppRewardCoupons via offline calcs, so the flow is more or less identical to the existing daml testing even in the case of RewardVersion_TrafficBasedAppRewards. And as long as the totalFeaturedAppRewardCoupons is being set properly it would work fine.
There was a problem hiding this comment.
We could pass in Optional Decimal to runNextIssuance and use that alongwith an assert on rewardConfig, to keep the logic in sync with the scala.
There was a problem hiding this comment.
Thanks. yes, good call.
@moritzkiefer-da : the code is now fully finished. If you find some time next week to do a final review, then that would be great.
Added "DO NOT MERGE" to title, as we want to merge this into the right feature branch once the implementation has progressed far enough.
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