Add UI support for fund manager blacklist and dev fund coupon mint delay - #6944
Add UI support for fund manager blacklist and dev fund coupon mint delay#6944zheli wants to merge 4 commits into
Conversation
10e7dd5 to
451c80d
Compare
451c80d to
5098ea2
Compare
b7ce42f to
5d29933
Compare
825b414 to
e4aaf32
Compare
… in frontend Signed-off-by: Zhe Li <linuxcity.jn@gmail.com>
…d mintAfter with tests Signed-off-by: Zhe Li <linuxcity.jn@gmail.com>
…ingDelay Signed-off-by: Zhe Li <linuxcity.jn@gmail.com>
Signed-off-by: Zhe Li <linuxcity.jn@gmail.com>
e4aaf32 to
860446c
Compare
meiersi-da
left a comment
There was a problem hiding this comment.
Thanks a lot. Direction looks good.
| val developmentFundCouponAmount = BigDecimal(SpliceUtil.damlDecimal(40.0)) | ||
| val expiresAt = CantonTimestamp.now().plus(Duration.ofDays(1)) | ||
| val reason = "Bob has contributed to the Daml repo" | ||
| val mintingDelay = Duration.ofSeconds(30) |
There was a problem hiding this comment.
When testing aspects related to timing we usually use a TimeBased integration test, which allowS changing the clock using advanceTime without busy waiting.
However, all the logic wrt minting delays is already tested at the Daml level. So what we need to test here is that a coupon with a minting delay gets eventually minted without raising unexpected errors.
I'd suggest we set a delay of 10s, which is long enough so it would result in a retry if the minting would be attempted, but short enough so its CI cost is not egregious. I'd then add 10s extra wait time also to the eventually that checks whether the coupon gets minted.
For bonus points, I'd change the minting code once to not respect the mint after, and check what kind of errors are raised in the logs (I expect retries); and then check again with the proper filtering that these are gone.
| .futureValue | ||
| .trigger[CollectRewardsAndMergeAmuletsTrigger] | ||
|
|
||
| archiveExistingUnclaimedDevelopmentFundCoupons() |
There was a problem hiding this comment.
this looks fishy. We usually do test isolation by relying on fresh parties being allocated for new test instances. So if you scope your queries wrt coupons being created to the test-instance-specific party, then you have proper isolation.
| clue("The coupon is left alone while its mintAfter is in the future") { | ||
| always(durationOfSuccess = 10.seconds) { | ||
| bobWalletClient | ||
| .listActiveDevelopmentFundCoupons() should have size 1 withClue "bob coupons before mintAfter" | ||
| bobWalletClient.balance().unlockedQty shouldBe bobBalanceBefore | ||
| } | ||
| CantonTimestamp | ||
| .now() | ||
| .isBefore(mintAfter) shouldBe true withClue "still before mintAfter" | ||
| } |
There was a problem hiding this comment.
I don't think we need to test this, as that's already tested at the Daml level.
| clue("The coupon is left alone while its mintAfter is in the future") { | |
| always(durationOfSuccess = 10.seconds) { | |
| bobWalletClient | |
| .listActiveDevelopmentFundCoupons() should have size 1 withClue "bob coupons before mintAfter" | |
| bobWalletClient.balance().unlockedQty shouldBe bobBalanceBefore | |
| } | |
| CantonTimestamp | |
| .now() | |
| .isBefore(mintAfter) shouldBe true withClue "still before mintAfter" | |
| } |
| } | ||
|
|
||
| clue("The coupon is collected once its mintAfter has passed") { | ||
| eventually(60.seconds) { |
There was a problem hiding this comment.
This is very expensive! Let's avoid it.
There was a problem hiding this comment.
With the above suggestion, this would be 30s, as the eventually has a 20s default and we add the 10s extra from the delay to get some extra protection from flakes.
| } | ||
| } | ||
|
|
||
| "not collect a development fund coupon before its mintAfter" in { implicit env => |
There was a problem hiding this comment.
For the same reason as explained above, I don't think we need an additional test, which increases both CI and maintenance cost. We just want to check that the coupons are ignored when their time is not due, and collected afterwards.
I'd suggest to restructure the existing test to do so as follows:
- create two dev fund coupons: one with
mintAfter = Noneand another one withmintAfter = Some 24h. - adjust https://github.com/bitdynamics-ab/splice/blob/860446c52b0247b6c3720bc7b7949300aad85fad/apps/app/src/test/scala/org/lfdecentralizedtrust/splice/integration/tests/WalletMintingDelegationTimeBasedIntegrationTest.scala#L591-L612 to expect to not collect the dev fund coupon that has a
mintAfter - add an
advanceTime(25h)afterwards and then check that the coupon withmintAfteris also collected.
|
|
||
| const result = buildAmuletRulesConfigFromChanges(changes); | ||
|
|
||
| expect(result.developmentFundManagerBlacklist).toEqual([]); |
There was a problem hiding this comment.
wdyt about setting it to null if it is emptied?
| developmentFundManagerBlacklist: null, | ||
| minDevelopmentFundMintingDelay: null, | ||
| // The frontend will never send null for developmentFundManagerBlacklist, an empty list is sent as `Some []` | ||
| // so that we can distinguish DSO that have not upgraded yet (they will have None for the field) |
There was a problem hiding this comment.
we do we need to distinguish that? My expectation was that forcing the upgrade happens by setting an actual minting delay.
| }); | ||
|
|
||
| // Happy | ||
| test('treats an absent minting delay as a zero delay and still prefills mintAfter', () => { |
There was a problem hiding this comment.
Note that this forces the recipient to have upgraded. We generally try to avoid this. How difficult would it be to not set mintAfter if no mintingDelay is set?
| /> | ||
| </Stack> | ||
|
|
||
| <Stack spacing={1} sx={{ flex: 1 }}> |
There was a problem hiding this comment.
Please add screenshots of all relevant states to the PR description to simplify review, as done here: #6387
Follow-up PR for #6793. It will fix the rest of the issue #6722, as well as #6904. It will close the milestone https://github.com/canton-network/splice/milestone/59
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./upgrade_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./lsu_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