Restrict amulet configs to not allow fees #3529
Conversation
moritzkiefer-da
force-pushed
the
cocreature/no-fees
branch
13 times, most recently
from
70beefa to
4c95333
Compare
moritzkiefer-da
force-pushed
the
cocreature/no-fees
branch
2 times, most recently
from
a30125c to
cbca1ff
Compare
| ) | ||
| } | ||
|
|
||
| "don't collect rewards if their collection is more expensive than they reward in amulets" in { |
There was a problem hiding this comment.
this check does nothing now that you don't pay fees
There was a problem hiding this comment.
We may want to consider adding a check for rewards that are too small probably just some configurable amount that roughly approximates traffic cost. But that seems orthogonal to this change.
There was a problem hiding this comment.
I guess we are lacking the protection wrt spending more traffic to collect the rewards than they are worth. That said: fine to postpone, as that goes away with the app rewards being accumulated directly by the DSO.
moritzkiefer-da
force-pushed
the
cocreature/no-fees
branch
from
cbca1ff to
3e42e23
Compare
| ) | ||
| } | ||
|
|
||
| "don't collect rewards if their collection is more expensive than they reward in amulets" in { |
There was a problem hiding this comment.
We may want to consider adding a check for rewards that are too small probably just some configurable amount that roughly approximates traffic cost. But that seems orthogonal to this change.
| ) | ||
| } | ||
|
|
||
| "generate app rewards correctly" in { implicit env => |
There was a problem hiding this comment.
Just doesn't make much sense to integration test this and we have daml script tests for it.
| } | ||
| } | ||
|
|
||
| "generate rewards for subscriptions" in { implicit env => |
There was a problem hiding this comment.
no point in integration testing this and it's kind of pointless as a test anyway, who cares if the DSO gets an app reward coupon …
|
@meiersi-da Finally got all the tests green so should be ready for a review. Not urgent |
meiersi-da
left a comment
meiersi-da
left a comment
There was a problem hiding this comment.
Very nice! What a monumental effort. Thank you! Glad it also burns 2k LoCs 🔥
...ala/org/lfdecentralizedtrust/splice/integration/tests/SplitwellFrontendIntegrationTest.scala
Show resolved
Hide resolved
| ) | ||
| } | ||
|
|
||
| "don't collect rewards if their collection is more expensive than they reward in amulets" in { |
There was a problem hiding this comment.
I guess we are lacking the protection wrt spending more traffic to collect the rewards than they are worth. That said: fine to postpone, as that goes away with the app rewards being accumulated directly by the DSO.
| ) | ||
| } | ||
|
|
||
| "generate app rewards correctly" in { implicit env => |
| } | ||
| } | ||
|
|
||
| "generate rewards for subscriptions" in { implicit env => |
| (unlockedBalance, _) <- userWalletStore.getAmuletBalanceWithHoldingFees( | ||
| round, | ||
| deductHoldingFees = deductHoldingFees, | ||
| deductHoldingFees = false, |
There was a problem hiding this comment.
nit: any reason to not remove the parameter altogether?
There was a problem hiding this comment.
let me take a quick look how easy it is to nuke it
moritzkiefer-da
force-pushed
the
cocreature/no-fees
branch
from
3e42e23 to
a9c119f
Compare
[ci] Signed-off-by: Moritz Kiefer <moritz.kiefer@purelyfunctional.org>
moritzkiefer-da
force-pushed
the
cocreature/no-fees
branch
from
a9c119f to
2896359
Compare
[ci]
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