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

[CU-2rb5h66] update reward pool #1346

Closed
wants to merge 8 commits into from
Closed

Conversation

andresmechali
Copy link
Contributor

Issue

https://app.clickup.com/t/2rb5h66

Checklist

  • I have updated the cargo docs to reflect changes made by this PR (if applicable)
  • I have updated the book/ to reflect changes made by this PR (if applicable)

@itsbobbyzz
Copy link

Task linked: CU-2rb5h66 Update reward pool

@vercel
Copy link

vercel bot commented Jul 19, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
pablo-staging ✅ Ready (Inspect) Visit Preview Jul 20, 2022 at 7:49AM (UTC)
pablo-storybook ❌ Failed (Inspect) Jul 20, 2022 at 7:49AM (UTC)
picasso-staging ✅ Ready (Inspect) Visit Preview Jul 20, 2022 at 7:49AM (UTC)
picasso-storybook ❌ Failed (Inspect) Jul 20, 2022 at 7:49AM (UTC)

@vercel vercel bot temporarily deployed to Preview – pablo-staging July 19, 2022 14:45 Inactive
@vercel vercel bot temporarily deployed to Preview – picasso-staging July 19, 2022 14:48 Inactive
@vercel vercel bot temporarily deployed to Preview – pablo-staging July 19, 2022 14:56 Inactive
@vercel vercel bot temporarily deployed to Preview – picasso-storybook July 19, 2022 15:00 Inactive
@vercel vercel bot temporarily deployed to Preview – pablo-storybook July 19, 2022 15:01 Inactive
@vercel vercel bot temporarily deployed to Preview – picasso-staging July 19, 2022 15:01 Inactive
Comment on lines 416 to 424
ensure!(
reward.max_rewards > reward.total_rewards + reward_update.amount,
Error::<T>::MaxRewardExceeded
);

reward.total_rewards = reward
.total_rewards
.safe_add(&reward_update.amount)
.map_err(|_| ArithmeticError::Overflow)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ensure!(
reward.max_rewards > reward.total_rewards + reward_update.amount,
Error::<T>::MaxRewardExceeded
);
reward.total_rewards = reward
.total_rewards
.safe_add(&reward_update.amount)
.map_err(|_| ArithmeticError::Overflow)?;
let new_total_rewards = reward
.total_rewards
.safe_add(&reward_update.amount)?;
ensure!(
reward.max_rewards >= new_total_rewards,
Error::<T>::MaxRewardExceeded
);
reward.total_rewards = new_total_rewards;

Copy link
Contributor Author

@andresmechali andresmechali Jul 20, 2022

Choose a reason for hiding this comment

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

No need to check for Overflow?

frame/staking-rewards/src/lib.rs Show resolved Hide resolved
Comment on lines 413 to 432
for (asset_id, reward) in inner_rewards.iter_mut() {
match reward_updates.get(asset_id) {
Some(reward_update) => {
ensure!(
reward.max_rewards > reward.total_rewards + reward_update.amount,
Error::<T>::MaxRewardExceeded
);

reward.total_rewards = reward
.total_rewards
.safe_add(&reward_update.amount)
.map_err(|_| ArithmeticError::Overflow)?;
// reward.total_rewards = reward.total_rewards
// .safe_add(elapsed_time * pool.reward_rate)
// .map_err(|_| ArithmeticError::Overflow)?;
reward.reward_rate = reward_update.reward_rate;
},
None => (),
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

As a note, this is possible without mutability. It would be much nicer if it was possible to iterate over a bounded map directly - I've opened paritytech/substrate#11866 to suggest an API to provide that functionality

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would be much nicer to read!

Timestamp::set_timestamp(200000);

let pool_init_config = get_default_reward_pool();
assert_ok!(StakingRewards::create_reward_pool(Origin::root(), pool_init_config));
Copy link
Contributor

Choose a reason for hiding this comment

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

Check that the RewardPoolCreated is emitted and get the pool_id from there (could extract this to a helper function in the tests, will surely be used elsewhere)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For what I see in another test, the pool_count() is used as pool_id. Will this be enough?

frame/staking-rewards/src/test/mod.rs Outdated Show resolved Hide resolved
@vercel vercel bot temporarily deployed to Preview – pablo-staging July 20, 2022 07:48 Inactive
@vercel vercel bot temporarily deployed to Preview – picasso-staging July 20, 2022 07:49 Inactive
@vercel vercel bot temporarily deployed to Preview – picasso-storybook July 20, 2022 07:49 Inactive
@vercel vercel bot temporarily deployed to Preview – pablo-storybook July 20, 2022 07:49 Inactive
@KaiserKarel KaiserKarel deleted the andres/update-reward-pool branch January 31, 2023 16:39
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.

3 participants