feat: add total reward for epoch for each user#204
Conversation
|
this looks nice! thanks for the PR. You are missing a migration to add the epoch field to the |
|
Ehi! you are still missing the migration, you need to add a folder here |
orm/migrations/2024-12-16-231256_add_epoch_pos_rewards_table/up.sql
Outdated
Show resolved
Hide resolved
|
you might also want to update the webserver? I mean, update/create a new endpoint |
| ALTER TABLE pos_rewards ADD COLUMN epoch INTEGER NOT NULL DEFAULT 0; | ||
| -- Now we can safely drop the default | ||
| ALTER TABLE pos_rewards ALTER COLUMN epoch DROP DEFAULT; No newline at end of file |
There was a problem hiding this comment.
While testing this, I ran into a problem where the pos_rewards table didn't have an updated UNIQUE constraint which incorporates the epoch column.
I pushed a commit that fixes that: 725f9c9
Feel free to add it to your branch also.
There was a problem hiding this comment.
Updated that here:
orm/src/pos_rewards.rs
Outdated
| raw_amount: BigDecimal::from_str(&reward.amount.to_string()) | ||
| .expect("Invalid amount"), | ||
| validator_id, | ||
| raw_amount: BigDecimal::from(reward.amount), |
There was a problem hiding this comment.
Note - this is assigning to raw_amount the decimal value of nam (i.e. 12.34567 not 1234567 unam) -- which is different than the previous behavior of the rewards crawler. I noticed it in my testing because the raw_amount column has type numeric(78,0) which only stores integer values, and the reward amounts were being truncated.
There was a problem hiding this comment.
fae5e1f all the webserver endpoints are in. basically it takes a delegator address, validator address and epoch and gives back the rewards. The validator address is used to find a validator id to look up the reward. Again, my rust sucks so let me know if there's anything off. |
|
Does this PR depend on namada-net/namada#4196? |
I'm 99% sure yes, but I'm also a noob to this codebase so @joel-u410 or @Fraccaman could confirm for sure. |
Hi @brentstone @neocybereth -- no, currently it does not, but after namada-net/namada#4196 is merged I would like this one to be updated to take advantage of it. Right now, in this branch, it only queries "current epoch" rewards, and just assigns the current epoch number to their records -- after namada-net/namada#4196 is merged I would want to change this to use the (I have a Draft commit where I've started working on that, including adding support for |
|
Just realized something here ... delete_claimed_rewards is now going to delete things it shouldn't. Instead of deleting, it should probably insert rows with Which will need to be distinguished somehow from the We'll probably need to add a column -- either |
|
@neocybereth I'm looking at the existing |
|
@joel-u410 epic, tahnk you for the heads up! What's your go to testing methodology here? I'm just shooting in the dark and YOLO-ing atm and surely there's gotta be a better way :D |
Yeaaaaa it was my rebase. I'm going to have to go through and piece the PR back together. |
|
do you still have the old tip of your branch in your |
omg you're a lifesaver! |
6d45623 to
7320744
Compare
|
@Fraccaman okay, we're back to clean. Everythings up-to-date against Anything else I need to do before getting approval? Shall I attempt another (hopefully successful :D) rebase against |
|
@neocybereth can you take a look at. theci failures? |
|
@Fraccaman sorry I've been busy with other work! I've fixed up all the CI issues. |
Co-authored-by: Tiago Carvalho <sugoiuguu@tfwno.gf>
Co-authored-by: Tiago Carvalho <sugoiuguu@tfwno.gf>
|
Fixed those up @sug0 |
|
@Fraccaman @sug0 just checking back in to see if these are good to merge? |

No description provided.