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

Reward payments 1 #76

Merged
merged 15 commits into from
Jun 28, 2023
Merged

Reward payments 1 #76

merged 15 commits into from
Jun 28, 2023

Conversation

ethanfrey
Copy link
Collaborator

Recreate from #69 after a mistaken merge and revert.

Enables rewards to be sent every epoch from consumer to producer and distributed to stakers.
Stakers can withdraw rewards to an account on the consumer chain (where they can be staked), with a message on the provider chain (which knows who staked)

Copy link
Collaborator

@maurolacy maurolacy left a comment

Choose a reason for hiding this comment

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

Looks good! Will take a look at tests later.

docs/ibc/Rewards.md Outdated Show resolved Hide resolved
docs/ibc/Rewards.md Show resolved Hide resolved
packages/apis/src/ibc/packet.rs Show resolved Hide resolved
packages/apis/src/ibc/packet.rs Show resolved Hide resolved
@@ -56,6 +69,13 @@ pub enum ConsumerPacket {
/// but when it is no longer a valid target to delegate to.
/// It contains a list of `valoper_address` to be removed
RemoveValidators(Vec<String>),
/// This is part of the rewards protocol
Distribute {
Copy link
Collaborator

@maurolacy maurolacy Jun 28, 2023

Choose a reason for hiding this comment

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

And this doesn't need to be transactional, because there's nothing to rollback here.

I wonder, if distribution fails on the provider for whatever reason, would that have an effect on subsequent distribution calls? I mean, couldn't we be distributing more / less than the right amount in some scenario?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That is a very good question. I made #81 to track this later. Not sure what the proper answer is right now. Not blocking merge but good to keep track of it.

contracts/provider/external-staking/src/ibc.rs Outdated Show resolved Hide resolved
contracts/provider/external-staking/src/contract.rs Outdated Show resolved Hide resolved
contracts/provider/external-staking/src/contract.rs Outdated Show resolved Hide resolved
@ethanfrey
Copy link
Collaborator Author

@maurolacy I addressed your comments in 0a7f563 if you want to check. And then rebased on main and resolved conflicts with other merges. I think this is more or less ready now.

@maurolacy
Copy link
Collaborator

@maurolacy I addressed your comments in 0a7f563 if you want to check. And then rebased on main and resolved conflicts with other merges. I think this is more or less ready now.

Will review tests, just for completeness.

Copy link
Collaborator

@maurolacy maurolacy left a comment

Choose a reason for hiding this comment

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

Some comments after a quick review on tests.

@@ -755,16 +752,14 @@ fn distribution() {
// 20 tokens for users[0]
// 30 tokens for users[1]
contract
.distribute_rewards(validators[0].to_owned())
.with_funds(&coins(50, STAR))
.test_distribute_rewards(validators[0].to_owned(), coin(50, STAR))
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is better / fixed now. Rewards to distribute are virtual here.

This test_ prefix is a little ugly, but makes things clear. Hopefully it will be gone when we're able to test this better.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, when both Sylvia and cw-multi-test have IBC support this should be much better.
Until then we just fake all ibc packet lifetimes.

I wouldn't assume this to improve til later Q3 (it is on Q3 roadmap though)

contracts/provider/external-staking/src/multitest.rs Outdated Show resolved Hide resolved
@ethanfrey ethanfrey merged commit 3e74dfd into main Jun 28, 2023
@ethanfrey ethanfrey deleted the reward-payments-1 branch June 28, 2023 09:05
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.

2 participants