-
Notifications
You must be signed in to change notification settings - Fork 17
feat: add total reward for epoch for each user #204
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
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated that here:
orm/src/pos_rewards.rs
Outdated
validator_id, | ||
raw_amount: BigDecimal::from(reward.amount), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 anoma/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 anoma/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 anoma/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 <[email protected]>
Co-authored-by: Tiago Carvalho <[email protected]>
Fixed those up @sug0 |
@Fraccaman @sug0 just checking back in to see if these are good to merge? |
No description provided.