[pos] Add epoch argument to vp::pos::rewards #4196

joel-u410 · 2024-12-18T16:03:25Z

Describe your changes

This adds an epoch argument to the rewards() function in vp::pos. It does require a RPC node update in order to work, but it's backwards compatible. Tested with previous build of namadan and results are same as before:

With node on main and client on this branch:

$ ./target/debug/namadac rewards --source tnam1qrtypl0yazfamk8ptmgp608msl8wzrkpmqwppc7w --validator tnam1qxsx2ezu89gx252kwwluqp7hadyp285tkczhaqg0
Current rewards available for claim: 4625.955882 NAM

With both client and node on this branch:

$ ./target/debug/namadac rewards --source tnam1qrtypl0yazfamk8ptmgp608msl8wzrkpmqwppc7w --validator tnam1qxsx2ezu89gx252kwwluqp7hadyp285tkczhaqg0
Current rewards available for claim: 4625.955882 NAM

$ ./target/debug/namadac rewards --source tnam1qrtypl0yazfamk8ptmgp608msl8wzrkpmqwppc7w --validator tnam1qxsx2ezu89gx252kwwluqp7hadyp285tkczhaqg0 --epoch 60
Rewards for epoch 60: 4625.955882 NAM

$ ./target/debug/namadac rewards --source tnam1qrtypl0yazfamk8ptmgp608msl8wzrkpmqwppc7w --validator tnam1qxsx2ezu89gx252kwwluqp7hadyp285tkczhaqg0 --epoch 59
Rewards for epoch 59: 3887.536527 NAM

$ ./target/debug/namadac rewards --source tnam1qrtypl0yazfamk8ptmgp608msl8wzrkpmqwppc7w --validator tnam1qxsx2ezu89gx252kwwluqp7hadyp285tkczhaqg0 --epoch 58
Rewards for epoch 58: 3148.755426 NAM

Checklist before merging

If this PR has some consensus breaking changes, I added the corresponding breaking:: labels
- This will require 2 reviewers to approve the changes
If this PR requires changes to the docs or specs, a corresponding PR is opened in the namada-docs repo
- Relevant PR if applies: (coming soon)
If this PR affects services such as namada-indexer or namada-masp-indexer, a corresponding PR is opened in that repo
- Relevant PR if applies: feat: backfill epoch rewards namada-indexer#235

codecov · 2024-12-18T16:38:42Z

Codecov Report

Attention: Patch coverage is 13.79310% with 50 lines in your changes missing coverage. Please review.

Project coverage is 74.42%. Comparing base (2ae305c) to head (89df61f).
Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/sdk/src/queries/vp/pos.rs	12.28%	50 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4196      +/-   ##
==========================================
- Coverage   74.46%   74.42%   -0.05%     
==========================================
  Files         342      342              
  Lines      107773   107826      +53     
==========================================
- Hits        80257    80252       -5     
- Misses      27516    27574      +58

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

joel-u410 · 2024-12-19T05:06:20Z

Testing that this behaves as expected for a validator that claimed rewards in epoch 51:

$ ./target/debug/namadac rewards --validator tnam1qyf80vw4e8aa2efuk4jsg9lpwxrntsj2dql4tssa --epoch 50
Rewards for epoch 50: 2.179855 NAM

$ ./target/debug/namadac rewards --validator tnam1qyf80vw4e8aa2efuk4jsg9lpwxrntsj2dql4tssa --epoch 51
Rewards for epoch 51: 4.363707 NAM

$ ./target/debug/namadac rewards --validator tnam1qyf80vw4e8aa2efuk4jsg9lpwxrntsj2dql4tssa --epoch 52
Rewards for epoch 52: 2.897536 NAM

joel-u410 · 2024-12-19T05:12:13Z

Of particular note, this code accounts for the rewards_counter when specifying an epoch by:

Subtracting the current counter that was included by query_reward_tokens
Adding the historical counter queried from storage at the height of epoch start

brentstone · 2024-12-20T06:57:43Z

Mind adding some unit tests here to demonstrate the behavior too @joel-u410?

joel-u410 · 2024-12-20T23:20:39Z

Mind adding some unit tests here to demonstrate the behavior too @joel-u410?

For sure, I can look into that.

…ter value

…st rewards_counter

…re as of beginning of epoch

joel-u410 · 2025-01-24T00:27:08Z

@brentstone @tzemanovic -- I've added a unit test that demonstrates querying for rewards with the epoch argument.

Please take a look: 8cb563f

Note that my test initially would not pass because the MockDB didn't provide a proper implementation for read_subspace_val_with_height . I added a very basic implementation (which does seem to work!) in this commit: 3c47541

tzemanovic · 2025-01-24T13:31:17Z

hi @joel-u410, thanks for the PR!

Do I understand correctly that you'd like to know the rewards distributed at the given epoch? The rewards query current returns unclaimed rewards, which accumulate and may be claimed at any point so I think there are some limitations with getting this information out of it. Specifically, when the rewards are claimed, the delegation bond amounts and validator commission rewards counter will go back to 0, but from this query endpoint it won't be very apparent what's happened. Because of this, I'm tempted to say that this functionality may be better built into an indexer, but if this is useful despite the limitations we could still add it, but I think we might need to better document how this work (I think one might expect that this returns the rewards obtained in the given epoch, but really it is accumulated unclaimed rewards)

tzemanovic · 2025-01-24T13:33:35Z

crates/sdk/Cargo.toml

@@ -162,6 +162,7 @@ namada_proof_of_stake = { path = "../proof_of_stake", default-features = false,
 namada_state = { path = "../state", features = ["testing"] }
 namada_storage = { path = "../storage", features = ["testing"] }
 namada_token = { path = "../token", features = ["testing", "masp"] }
+namada_trans_token = { path = "../trans_token" }


this shouldn't be needed - the symbols used are re-exported from namada_token

tzemanovic · 2025-01-24T13:34:22Z

crates/sdk/src/queries/vp/pos.rs

+                    _,
+                    namada_parameters::Store<_>,
+                    governance::Store<_>,
+                    namada_trans_token::Store<_>,


Suggested change

namada_trans_token::Store<_>,

namada_token::Store<_>,

tzemanovic · 2025-01-24T13:34:35Z

crates/sdk/src/queries/vp/pos.rs

+            StorageWrite::write(
+                &mut client.state,
+                &storage::Key::from(
+                    namada_trans_token::storage_key::balance_key(


Suggested change

namada_trans_token::storage_key::balance_key(

namada_token::storage_key::balance_key(

tzemanovic · 2025-01-24T13:34:44Z

crates/sdk/src/queries/vp/pos.rs

+            namada_proof_of_stake::bond_tokens::<
+                _,
+                governance::Store<_>,
+                namada_trans_token::Store<_>,


Suggested change

namada_trans_token::Store<_>,

namada_token::Store<_>,

tzemanovic · 2025-01-24T13:37:53Z

crates/sdk/src/args.rs

@@ -101,6 +101,8 @@ pub trait NamadaTypes: Clone + std::fmt::Debug {
    type MaspIndexerAddress: Clone + std::fmt::Debug;
    /// Represents a block height
    type BlockHeight: Clone + std::fmt::Debug;
+    /// Represents an epoch
+    type Epoch: Clone + std::fmt::Debug;


this addition is not needed - you can refer to the concrete Epoch type from both impls (I see you probably followed what's been done and there are already bunch of types here that don't need to be generic)

tzemanovic · 2025-01-24T13:38:53Z

crates/sdk/src/queries/vp/pos.rs

+    // querying by height.
+    // TODO: this might not be valid if rewards have been claimed in the current
+    // epoch? (because the rewards_counter would be removed from
+    // storage until the next epoch)


yeah, that's right :/

tzemanovic · 2025-01-24T13:40:33Z

crates/sdk/src/queries/vp/pos.rs

+            let native_token = client.state.get_native_token().unwrap();
+            StorageWrite::write(
+                &mut client.state,
+                &storage::Key::from(


this is already a storage key

Fraccaman requested a review from tzemanovic December 19, 2024 12:27

brentstone added PoS client labels Dec 21, 2024

joel-u410 mentioned this pull request Dec 23, 2024

feat: backfill epoch rewards anoma/namada-indexer#235

Draft

brentstone mentioned this pull request Jan 2, 2025

feat: add total reward for epoch anoma/namada-indexer#204

Open

joel-u410 added 10 commits January 23, 2025 18:20

[pos] Add epoch argument to vp::pos::rewards

d94ec05

[pos] when querying rewards by epoch, use the historical rewards_coun…

abcdefa

…ter value

refactor: use existing storage_value query function for looking up pa…

e1cf919

…st rewards_counter

cargo fmt

a6257eb

slight wording change: for -> at to make it more clear that rewards a…

bd78cba

…re as of beginning of epoch

refactor: Amount::from(0) -> Amount::zero()

1f5dd5a

changelog: add

7937b91

[vscode] Add namada_sdk unit test debug launch definition

cb29c9d

enhancement: implement read_subspace_val_with_height for mockdb

3c47541

[pos] Add unit test for vp::pos::rewards query with epochs

8cb563f

joel-u410 force-pushed the joel/rewards-at-past-epochs branch from 89df61f to 8cb563f Compare January 24, 2025 00:23

tzemanovic reviewed Jan 24, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[pos] Add epoch argument to vp::pos::rewards #4196

[pos] Add epoch argument to vp::pos::rewards #4196

joel-u410 commented Dec 18, 2024 •

edited

Loading

codecov bot commented Dec 18, 2024 •

edited

Loading

joel-u410 commented Dec 19, 2024

joel-u410 commented Dec 19, 2024 •

edited

Loading

brentstone commented Dec 20, 2024

joel-u410 commented Dec 20, 2024

joel-u410 commented Jan 24, 2025 •

edited

Loading

tzemanovic commented Jan 24, 2025

tzemanovic Jan 24, 2025

tzemanovic Jan 24, 2025

tzemanovic Jan 24, 2025

tzemanovic Jan 24, 2025

tzemanovic Jan 24, 2025

tzemanovic Jan 24, 2025

tzemanovic Jan 24, 2025

	namada_trans_token::storage_key::balance_key(
	namada_token::storage_key::balance_key(

[pos] Add epoch argument to vp::pos::rewards #4196

Are you sure you want to change the base?

[pos] Add epoch argument to vp::pos::rewards #4196

Conversation

joel-u410 commented Dec 18, 2024 • edited Loading

Describe your changes

Checklist before merging

codecov bot commented Dec 18, 2024 • edited Loading

Codecov Report

joel-u410 commented Dec 19, 2024

joel-u410 commented Dec 19, 2024 • edited Loading

brentstone commented Dec 20, 2024

joel-u410 commented Dec 20, 2024

joel-u410 commented Jan 24, 2025 • edited Loading

tzemanovic commented Jan 24, 2025

tzemanovic Jan 24, 2025

Choose a reason for hiding this comment

tzemanovic Jan 24, 2025

Choose a reason for hiding this comment

tzemanovic Jan 24, 2025

Choose a reason for hiding this comment

tzemanovic Jan 24, 2025

Choose a reason for hiding this comment

tzemanovic Jan 24, 2025

Choose a reason for hiding this comment

tzemanovic Jan 24, 2025

Choose a reason for hiding this comment

tzemanovic Jan 24, 2025

Choose a reason for hiding this comment

joel-u410 commented Dec 18, 2024 •

edited

Loading

codecov bot commented Dec 18, 2024 •

edited

Loading

joel-u410 commented Dec 19, 2024 •

edited

Loading

joel-u410 commented Jan 24, 2025 •

edited

Loading