Skip to content
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

Fix get-entitled-stacking-reward to pass value after shutdown #200

Merged
merged 4 commits into from
May 5, 2022

Conversation

whoabuddy
Copy link

This PR fixes #196 by adding an extra check to calculate the reward if the contract is shut down.

The logic here is that after a shutdown call is made, mining/stacking are immediately disabled. This means the cycle is effectively over as no new contributions will be made to stackers.

Because of that, after an upgrade, stackers should be able to claim their CityCoins and their portion of the STX contributed by miners within that cycle.

It's a simple change, but was hard to test given our configuration and is something we'll have to revisit for future upgrade tests.

This commit can be reverted or the changes removed when done, but will show that both the core-v2 tests still pass after the change, and adds a new test to ensure both CityCoins and STX are transferred when claiming in the current cycle.

This will also provide an artifact of the testing with the related github action.
@codecov
Copy link

codecov bot commented May 5, 2022

Codecov Report

Merging #200 (5f2ae9a) into develop (abfa931) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop     #200   +/-   ##
========================================
  Coverage    98.39%   98.39%           
========================================
  Files           22       22           
  Lines         4416     4418    +2     
========================================
+ Hits          4345     4347    +2     
  Misses          71       71           
Impacted Files Coverage Δ
contracts/cities/mia/local/miamicoin-core-v2.clar 97.02% <100.00%> (+<0.01%) ⬆️
...acts/cities/nyc/local/newyorkcitycoin-core-v2.clar 97.02% <100.00%> (+<0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update abfa931...5f2ae9a. Read the comment docs.

@whoabuddy
Copy link
Author

In commit 1ac3984 we can see that if we apply the changes to the core-v1 contracts, the tests show that both STX/CityCoins are returned after a contract shutdown, and all other tests continue to operate as expected. This should be safe to merge in before the next deployment!

Will revert these changes to v1 contracts but leave them in V2 on the next commit, then PR will be ready for review.

From the related GitHub Action to the changes in 859791b and 1ac3984:

MIA Reference

(if (and (not (var-get isShutdown))
(or (<= currentCycle targetCycle) (is-eq u0 userStackedThisCycle)))
;; the contract is not shut down and
;; this cycle hasn't finished
;; or stacker contributed nothing
u0
;; (totalUstxThisCycle * userStackedThisCycle) / totalStackedThisCycle
(/ (* totalUstxThisCycle userStackedThisCycle) totalStackedThisCycle)
)

it("claim-stacking-reward() succeeds in v1 with current reward cycle after shutdown and returns CityCoins and STX", () => {
// this confirms a the bug fix for `get-entitled-stacking-reward`
// arrange
const targetCycle = 16;
// act
const block = chain.mineBlock([
core.claimStackingReward(targetCycle, user2)
]);
const receipt = block.receipts[0];
// assert
receipt.result.expectOk().expectBool(true);
assertEquals(receipt.events.length, 2);
receipt.events.expectFungibleTokenTransferEvent(
1000,
core.address,
user2.address,
"miamicoin"
);
receipt.events.expectSTXTransferEvent(
35000,
core.address,
user2.address
);
});

* [MiamiCoin Upgrade v1-v2] claim-stacking-reward() succeeds in v1 with current reward cycle after shutdown and returns CityCoins without returning STX ... ignored (0ms)
* [MiamiCoin Upgrade v1-v2] claim-stacking-reward() succeeds in v1 with current reward cycle after shutdown and returns CityCoins and STX ... ok (1328ms)
* [MiamiCoin Upgrade v1-v2] claim-stacking-reward() succeeds in v1 with current reward cycle after shutdown and returns CityCoins and STX ... ok (1323ms)

NYC Reference

(if (and (not (var-get isShutdown))
(or (<= currentCycle targetCycle) (is-eq u0 userStackedThisCycle)))
;; the contract is not shut down and
;; this cycle hasn't finished
;; or stacker contributed nothing
u0
;; (totalUstxThisCycle * userStackedThisCycle) / totalStackedThisCycle
(/ (* totalUstxThisCycle userStackedThisCycle) totalStackedThisCycle)
)

it("claim-stacking-reward() succeeds in v1 with current reward cycle after shutdown and returns CityCoins and STX", () => {
// this confirms a the bug fix for `get-entitled-stacking-reward`
// arrange
const targetCycle = 10;
// act
const block = chain.mineBlock([
core.claimStackingReward(targetCycle, user2)
]);
const receipt = block.receipts[0];
// assert
receipt.result.expectOk().expectBool(true);
assertEquals(receipt.events.length, 2);
receipt.events.expectFungibleTokenTransferEvent(
1000,
core.address,
user2.address,
"newyorkcitycoin"
);
receipt.events.expectSTXTransferEvent(
35000,
core.address,
user2.address
);
});

* [NewYorkCityCoin Upgrade v1-v2] claim-stacking-reward() succeeds in v1 with current reward cycle after shutdown and returns CityCoins without returning STX ... ignored (0ms)
* [NewYorkCityCoin Upgrade v1-v2] claim-stacking-reward() succeeds in v1 with current reward cycle after shutdown and returns CityCoins and STX ... ok (1349ms)
* [NewYorkCityCoin Upgrade v1-v2] claim-stacking-reward() succeeds in v1 with current reward cycle after shutdown and returns CityCoins and STX ... ok (1337ms)

@whoabuddy whoabuddy requested a review from LNow May 5, 2022 20:39
Copy link

@LNow LNow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job! 🥇

@whoabuddy whoabuddy merged commit 7375699 into develop May 5, 2022
@whoabuddy whoabuddy deleted the fix/entitled-stacking-reward branch May 5, 2022 21:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants