Batch rewards distribution #125

uint · 2023-09-14T19:24:49Z

Closes #77

There's this comment in the ticket:

This requires much more complex withdraw logic in the virtual-staking contract, and first we need to be able to cover that with unit tests.

Given that the ticket talks about batching distribution and not withdrawing, I don't understand the need for changing withdraw logic. I feel like maybe there's an implication withdrawing could be optimized too, but I have no way of knowing for sure what was meant. If I'm missing something or there's further work to be done, please let me know.

uint · 2023-09-15T09:21:27Z

This is only missing tests on provider site, but I'm going to first try and add better abstractions to provider tests.

uint · 2023-09-18T19:48:39Z

@maurolacy I've noticed non-batch distribution returns no error when an invalid validator is provided. Is that intended? Do we want the same behavior for batch?

maurolacy · 2023-09-19T05:52:43Z

@maurolacy I've noticed non-batch distribution returns no error when an invalid validator is provided. Is that intended? Do we want the same behavior for batch?

That's OK, because that validators list comes from the blockchain. Take a look at the virtual-staking contract, the handle_epoch and reply_rewards methods gather the rewards automatically from the blockchain.

maurolacy

Good start. Please adjust / remove tests, as invalid validators shouldn't be possible.

packages/apis/src/ibc/packet.rs

contracts/consumer/converter/src/contract.rs

contracts/provider/external-staking/src/ibc.rs

contracts/provider/external-staking/src/contract.rs

maurolacy · 2023-09-19T06:30:31Z

Given that the ticket talks about batching distribution and not withdrawing, I don't understand the need for changing withdraw logic. I feel like maybe there's an implication withdrawing could be optimized too, but I have no way of knowing for sure what was meant. If I'm missing something or there's further work to be done, please let me know.

What's missing is to send a DistributeBatch message from the virtual-staking contract to the converter, instead of a bunch of Distribute messages.

I'm realising now that this might be complex to do. Let's discuss details offline.

maurolacy · 2023-09-19T07:00:33Z

@maurolacy I've noticed non-batch distribution returns no error when an invalid validator is provided. Is that intended? Do we want the same behavior for batch?

That's OK, because that validators list comes from the blockchain. Take a look at the virtual-staking contract, the handle_epoch and reply_rewards methods gather the rewards automatically from the blockchain.

What we can do to err on the safe side, is to add a sender check to the distribute_reward / distribute_rewards handlers, so that the only authorised caller is the virtual-staking contract.

That way we'll prevent malicious actors on the Consumer sending small amounts with invalid validators to distribute, to mess with our mesh security impl.

Update: That, or check that the validator is valid. This is related to #123 by the way. Let's discuss offline.

uint · 2023-09-19T18:52:35Z

@maurolacy How do we test the code in virtual-staking currently?

maurolacy · 2023-09-20T11:54:06Z

@maurolacy How do we test the code in virtual-staking currently?

Good question. Short answer is, we don't have a good way to test some parts of it at this level at the moment. That's why it's mostly untested.
Feel free to write some unit tests if you think it's worth it / useful. But some parts, like the reply handler behaviour, are difficult to test in this setup.

By the way, now that Sylvia has support for custom queries, we can test the sudo entry point better (there's a FIXME in the code for that). But this is another issue.

On the bright side, the rewards gathering and distribution code was tested as part of mesh-security integration on a devnet, and it works.

uint · 2023-09-20T16:20:50Z

@maurolacy How do we test the code in virtual-staking currently?

Good question. Short answer is, we don't have a good way to test some parts of it at this level at the moment. That's why it's mostly untested. Feel free to write some unit tests if you think it's worth it / useful. But some parts, like the reply handler behaviour, are difficult to test in this setup.

I have ideas about how to test this, but they might require some refactoring and bloat this PR even more. I think I'd be more comfortable getting this merged and starting a new PR.

maurolacy · 2023-09-21T06:24:20Z

Looks good. Is this ready for (final) review? Can you mark it as such? Thanks.

uint · 2023-09-21T07:51:13Z

Looks good. Is this ready for (final) review? Can you mark it as such? Thanks.

I was going to give it a once-over before doing that.

maurolacy

Reviewed the virtual-staking integration and found some issues. Will review the rest later.

contracts/consumer/virtual-staking/src/contract.rs

maurolacy

Looks better, but I think that there are still a couple of issues with the main logic.

contracts/consumer/virtual-staking/src/contract.rs

uint · 2023-09-27T09:18:44Z

I have ideas about how to test this, but they might require some refactoring and bloat this PR even more. I think I'd be more comfortable getting this merged and starting a new PR.

Since the tests didn't really require refactoring (just a bunch of new code), and the lack of them makes this PR development massively painful, I'm going to merge the tests here after all. Sorry about the LoC!

…tability Test `virtual-staking` `reply_rewards`

maurolacy

LGTM. Nice tests!

maurolacy · 2023-09-28T07:24:10Z

contracts/consumer/virtual-staking/src/contract.rs

-            funds: vec![reward],
+        let new_reward_amount = total - BATCH.total(deps.storage)?;
+
+        let all_rewards = |storage| {


This logic is now more complex than before. 🙂

OK, I trust this is correct, based on the tests below.

Nice way of testing it, by the way.

uint added 2 commits September 14, 2023 16:34

converter: batch rewards message

78e9254

provider: handle batch reward distribution

0497287

uint changed the title ~~converter: batch rewards distribution~~ Batch rewards distribution Sep 15, 2023

uint added 5 commits September 18, 2023 11:48

provider: abstract away creating an app with balances

22134b3

provider: abstract away activating validators

0ca318c

provider tests: abstract away staking

36df343

provider: more test utils

9c4d9b6

provider: batch reward distribution tests

b47b80b

uint force-pushed the 77-batch-distribute-rewards branch from cfe2704 to b47b80b Compare September 18, 2023 19:45

maurolacy reviewed Sep 19, 2023

View reviewed changes

uint added 3 commits September 19, 2023 18:41

provider: remove invalid validator test

646bd49

style

61123f3

virtual-staking: batch distribute messages

a56fff0

uint added 3 commits September 19, 2023 21:10

converter: virtual_staking auth

5abf297

style

da51bb4

converter: test unauthorized scenarios

32dce0d

ext-stk: give test utils their own module

8c102c4

uint force-pushed the 77-batch-distribute-rewards branch from 81a5b22 to 8c102c4 Compare September 21, 2023 08:12

uint marked this pull request as ready for review September 21, 2023 08:18

uint requested a review from maurolacy September 21, 2023 08:18

maurolacy reviewed Sep 25, 2023

View reviewed changes

contracts/consumer/virtual-staking/src/contract.rs Outdated Show resolved Hide resolved

contracts/consumer/virtual-staking/src/contract.rs Show resolved Hide resolved

virt-stk: fix reply_rewards bugs

60f8280

uint requested a review from maurolacy September 25, 2023 09:24

uint added 7 commits September 25, 2023 14:00

virt-stk: reply_rewards - fix edge case

49e26ab

virt-stk: reply_rewards tests

9b3ec02

virt-stk: refactor tests

dca8913

virt-stk: add test for when the last push is 0

4bbc757

virt-stk: test utils bugfix

9de5fd7

virt-stk: one more reply_rewards test

55c6219

lints

da3aeb8

maurolacy reviewed Sep 27, 2023

View reviewed changes

contracts/consumer/virtual-staking/src/contract.rs Outdated Show resolved Hide resolved

contracts/consumer/virtual-staking/src/contract.rs Outdated Show resolved Hide resolved

contracts/consumer/virtual-staking/src/contract.rs Outdated Show resolved Hide resolved

uint and others added 3 commits September 27, 2023 11:29

Merge pull request #131 from osmosis-labs/improve-virtual-staking-tes…

7da61b5

…tability Test `virtual-staking` `reply_rewards`

virt-stk: Fix repeatedly distributing rewards

a73fca4

virt-stk: improve reply_rewards logic

e35ce75

uint force-pushed the 77-batch-distribute-rewards branch from 6bd454e to e35ce75 Compare September 27, 2023 20:45

uint requested a review from maurolacy September 27, 2023 20:52

maurolacy approved these changes Sep 28, 2023

View reviewed changes

uint merged commit d501412 into main Sep 28, 2023
2 checks passed

maurolacy deleted the 77-batch-distribute-rewards branch October 12, 2023 11:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Batch rewards distribution #125

Batch rewards distribution #125

uint commented Sep 14, 2023 •

edited

Loading

uint commented Sep 15, 2023

uint commented Sep 18, 2023

maurolacy commented Sep 19, 2023

maurolacy left a comment

maurolacy commented Sep 19, 2023

maurolacy commented Sep 19, 2023 •

edited

Loading

uint commented Sep 19, 2023

maurolacy commented Sep 20, 2023 •

edited

Loading

uint commented Sep 20, 2023

maurolacy commented Sep 21, 2023

uint commented Sep 21, 2023

maurolacy left a comment •

edited

Loading

maurolacy left a comment

uint commented Sep 27, 2023 •

edited

Loading

maurolacy left a comment

maurolacy Sep 28, 2023

Batch rewards distribution #125

Batch rewards distribution #125

Conversation

uint commented Sep 14, 2023 • edited Loading

uint commented Sep 15, 2023

uint commented Sep 18, 2023

maurolacy commented Sep 19, 2023

maurolacy left a comment

Choose a reason for hiding this comment

maurolacy commented Sep 19, 2023

maurolacy commented Sep 19, 2023 • edited Loading

uint commented Sep 19, 2023

maurolacy commented Sep 20, 2023 • edited Loading

uint commented Sep 20, 2023

maurolacy commented Sep 21, 2023

uint commented Sep 21, 2023

maurolacy left a comment • edited Loading

Choose a reason for hiding this comment

maurolacy left a comment

Choose a reason for hiding this comment

uint commented Sep 27, 2023 • edited Loading

maurolacy left a comment

Choose a reason for hiding this comment

maurolacy Sep 28, 2023

Choose a reason for hiding this comment

uint commented Sep 14, 2023 •

edited

Loading

maurolacy commented Sep 19, 2023 •

edited

Loading

maurolacy commented Sep 20, 2023 •

edited

Loading

maurolacy left a comment •

edited

Loading

uint commented Sep 27, 2023 •

edited

Loading