Query staking changelog #130
Conversation
|
Can we add this to Otherwise, LGTM. |
| deps: Deps, | ||
| address: String, | ||
| start_height: u64, | ||
| end_height: u64, |
There was a problem hiding this comment.
Best to make the above Optional for better dev UX
There was a problem hiding this comment.
Agree with using optional values here. You can also consider adding pagination / continuation, as the changelog can run very large with time.
There was a problem hiding this comment.
On second thought, this is already a form of pagination, in that you are providing the range.
So, this is OK as it is. It's when / if introducing optional values that it can have unexpected issues with the number of entries. Maybe a start_after, limit approach like in cw-plus would be better then.
| for (h, a) in changes { | ||
| app.update_block(|b| b.height = h); | ||
| stake_tokens(&mut app, &staking_addr, &cw20_addr, info.clone(), a).unwrap(); | ||
| } | ||
|
|
||
| let expected = vec![ | ||
| (start_height + 300, Uint128::new(300)), | ||
| (start_height + 200, Uint128::new(200)), | ||
| (start_height + 100, Uint128::new(100)), | ||
| (start_height, Uint128::new(0)), | ||
| ]; | ||
|
|
||
| let result = query_changelog(&app, &staking_addr, ADDR1, start_height, start_height + 300); | ||
| assert_eq!(result, expected); |
| deps: Deps, | ||
| address: String, | ||
| start_height: u64, | ||
| end_height: u64, |
There was a problem hiding this comment.
Agree with using optional values here. You can also consider adding pagination / continuation, as the changelog can run very large with time.
| let address = &deps.api.addr_validate(&address)?; | ||
| let min_bound = Bound::inclusive_int(start_height); | ||
| // This bound is exclusive as we manually add the first entry to ensure we always know the final value | ||
| let max_bound = Bound::exclusive_int(end_height); |
There was a problem hiding this comment.
If there was an update precisely at end_height, you'll be missing its old value with this.
| let final_balance = STAKED_BALANCES | ||
| .may_load_at_height(deps.storage, address, end_height)? | ||
| .unwrap_or_default(); | ||
| let mut result = vec![(end_height, final_balance)]; |
There was a problem hiding this comment.
This is the current value at end_height. So, you are mixing old with current values here. This obviously depends on your use case, and it's acceptable if you know about this convention, but still, I think it's not entirely clear / consistent.
Check below for a suggestion on how to make this consistent.
| None => (h, Uint128::zero()), | ||
| Some(old) => (h, old), |
There was a problem hiding this comment.
Just a detail, but given that you are running this map, why not using h-1 here instead of h?
That way, those would be the current balances at h-1 (consistent with the balance at end_height). Every element of the vector will have the current balance in the range of the previous h+1, its h.
The nice thing about this is that all the entries will be consistent. It will also allow you to use an inclusive bound for end_height, without repeated values and without potential loss of information.
Finally, you can use saturating_sub here, to handle the rare (or perhaps, impossible?) case of h being zero.
There was a problem hiding this comment.
So this is all a bit kludge, but I think the current implementation is consistent with the data the caller would expect.
First, snapshotmap.may_load_at _height returns the value at the beginning of the block, not the value after any updates that occur. For example, lets say at height 20 the value is changed from 1 to 2. may_load_at_height will return 1 for height 20. We use this function when querying the staking balance at a given height, so should be consistent with that query function. Any staking rewards should be calculated with balance at the beginning of the block. So with the example of (h, old), we actually do want to use the old value at height h.
Second, I am using an exclusive bound at end height because we want to ensure we have a final entry. In majority of cases there will not be a changelog entry at the end height. In this case we need to know the balance from the end block to the first change in the array. To do this we simply manually add an entry to the change log for the balance at the end height. This method still work if there is a change at the end block and, in this case, returns the same result as using an inclusive bound.
There was a problem hiding this comment.
If may_load_at_height returns the previous value at height end_height, then everything is consistent, yes.
This is a fun one that takes advantage of a fresh upstream change to
cw-storage-plushttps://github.com/CosmWasm/cw-plus/pull/622/files.The motivation of this PR is to allow a reward contract to query the change log of a specific address's staking balance in order to efficiently calculate block based rewards. Some weirdness in how this result is returned. Lets walk through an example.
Meow recently received his crabs airdrop and can't wait to get his staking rewards. He stakes 10 crabs at block 10, 10 crabs at block 20, and 10 crabs at block 30. At block 100 we check the change log for blocks 0 to 30 and receive the following vector.
So the result is in reverse chronological order with the value being the balance for the earlier blocks. A little awkward, but this is due to the underlying changelog in the data structure storing the old value. It is then reversed as this is the natural order to calculate block based rewards with the old values. We manually add the most recent value at the end of the requested data range. In this example this lets us know the balance between blocks 100 and 30. It is also important to note that in all cases in this contract if you query a block where the balance changes, we return the previous value not the updated value. For example, staked balance at block 30 will be 20 not 30. This is how the underlying snapshot map is implemented and imo it is acceptable behavior.
This PR is also pulling the
cw-storage-plusrepo straight from github. This is unblocking us to keep building and I assume we will get an official release in a resonable amount of time.