-
Notifications
You must be signed in to change notification settings - Fork 33
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
[WIP] Feat staker v2 #63
Conversation
…support Q128.128 FP. Added cumulative seconds per total staked history value, and getter for that value Added method that allows to calculate how much staked seconds would staked token amount generate in between 2 timestamps: `calculate_staked_seconds_for_amount_between` Added tiny amount of tests
* removed staked seconds calculation * added seconds per total staked test and added logic for calculation of that value when nothing is staked
@moodysalem Could you, please, review? |
src/staker.cairo
Outdated
#[derive(Drop, Serde, starknet::Store)] | ||
struct StakingLogRecord { | ||
timestamp: u64, | ||
total_staked: u128, | ||
cumulative_seconds_per_total_staked: UFixedPoint, | ||
} |
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.
todo: store pack the timestamp and total staked into one felt252
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.
My bad. Thank you for catching that. I should've used felt252 to store in a single slot.
src/utils/fp.cairo
Outdated
} | ||
|
||
pub(crate) impl FixedPointIntoU256 of Into<UFixedPoint, u256> { | ||
fn into(self: UFixedPoint) -> u256 { self.value.try_into().unwrap() } |
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.
this really should be a try_into
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.
Good point.
src/utils/fp.cairo
Outdated
pub(crate) value: u512 | ||
} | ||
|
||
pub impl UFixedPointStorePacking of StorePacking<UFixedPoint, u256> { |
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.
a u256 takes two slots, the fixed point number in this case is a 64.128 which is 192 bits which can fit in a single felt. would recommend not creating a type just for this unless it's specifically a FixedPoint64x128 type, but we haven't created special types elsewhere
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.
Rookie mistake.
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.
would recommend not creating a type just for this unless it's specifically a FixedPoint64x128 type, but we haven't created special types elsewhere
I though that having a small abstraction would not hurt, especially as it introduces zero cost. I understand that addition and subtraction are not needed and probably won't, but those seemed to easy to implement to omit those. Also having that as an abstract separate value allowed me for better testing. And having tests in place it was easy to refactor things. I just finished optimising the code, improving the storage and added special convenience method for creation of that value. This value is now FixedPoint124x128 (fits one felt252 exactly).
well. almost finished (fixing tests atm)
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.
But you know. I'll try to make it a minimal size.
src/staker.cairo
Outdated
#[derive(Drop, Serde, starknet::Store)] | ||
struct StakingLogRecord { | ||
timestamp: u64, | ||
total_staked: u128, |
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.
probably better to store total_staked * seconds elapsed
accumulator so we can get time weighted average total staked over a period. it makes the value ~32 bits wider but you can still always get the exact total staked amount at a particular time by using the time weighted average over 1 second
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.
the reason is that the time weighted total staked is what counts as the total voting weight, not the total staked at a particular second
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.
I finally got myself some piece of calm and though on that comment.
Short
I would suggest to store both, total_staked + weighed sum staked fields
Long
TWA - time weighted average.
We won't be able to calculate total_staked as TWA over 1 second.
While I was writing this I figured out that we still can calculate total_staked. But not for every case.
Let's imagine:
TS - timestamp
TWS - weighed sum staked, numenator of TWA
AMT - total staked
SPTS - seconds per total staked
Let's assume our stakes changes like that:
TS | AMT * | TWS | SPTS |
---|---|---|---|
0 | 100 | 0 | 0 |
1 | 200 | 100 | 0.01 |
5 | 300 | 900 | 0.03 |
7 | 0 | 1500 | 0.03(6) |
- means that value is not stored in log and has to be calculated always
I'll use column name as a function that takes TS to search for row and returns value of column for that row TWS(0) = 0, TWS(5) = 900.
Let's imagine I want to calculate TWA at point 4:
As AMT is erased I'll need to calculate:
AMT(4) = (TWS(5) - TWS(1)) / (TS(5) - TS(1))
then I'll be able:
TWA(4) = (TWS(1) + AMT(4)*(4 - TS(1))) / 4
Ok. Good. This will work.
But if I need to calculate TWA(10) - the only way to figure out AMT(7) will be through SPTS.
I'll need to AMT(7) = (TS(7) - TS(5))/(SPTS(7) - SPTS(5))
And there will be a corner case for all the TSs < 0.
src/staker.cairo
Outdated
@@ -104,6 +122,8 @@ pub mod Staker { | |||
amount_delegated: Map<ContractAddress, u128>, | |||
delegated_cumulative_num_snapshots: Map<ContractAddress, u64>, | |||
delegated_cumulative_snapshot: Map<ContractAddress, Map<u64, DelegatedSnapshot>>, | |||
|
|||
staking_log: Vec<StakingLogRecord>, |
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.
is there a limit to the stored vector length? we use maps elsewhere, e.g. delegated_cumulative_snapshot
and delegated_cumulative_num_snapshots
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.
I see. I don't think that size is the problem. But yeah seems that using Maps consumes less gas. Will do a bit of a research. Thank you for pointing that out!
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.
I've done some research on that comment about representing Array of log records as a Map instead of Vec. Vec is actually similar to Map, with one distinction, it tracks count of elements. As we have append only log we don't need to worry about reindexing issues. Changing to Map will ruin the possibility to quickly find log record <= to the timestamp provided. The size restriction for Vec is 1 felt, which is 252 bits. I've calculated and even if we store data with 1 sec resolution we will have enough space to store data for 6.3*10^79 years. Which is actually ok. I think. I mean. Even if it'll become a problem I'll be dead by the time 8 ).
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.
I've checked some more. While the documentation says that whole felt252 is reserved for Vector index, Vec api only allow u64 as index, which still allows for 573 * 10**9 years of data for second resolution.
src/staker.cairo
Outdated
let total_staked = if is_add { | ||
// overflow check | ||
assert(last_record.total_staked + amount >= last_record.total_staked, 'BAD AMOUNT'); | ||
last_record.total_staked + amount | ||
} else { | ||
// underflow check | ||
assert(last_record.total_staked >= amount, 'BAD AMOUNT'); | ||
last_record.total_staked - 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.
if you only store cumulatives you can avoid this write in case where the timestamp is not changed
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.
Makes sense.
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.
Simplified while moving staker_log to separate file.
|
||
set_block_timestamp(5000); // 5 seconds passed | ||
|
||
assert(staker.get_staked(token_owner, delegatee) == 2, 'Something went wrong'); |
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.
you should use the assert_eq! macro btw, no need to write error messages
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.
I think that assertion messages lack line numbers, that is why I decided to stick with custom messages. But sure, no probs, will fix.
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.
usually wouldn't matter because the assertion lines are never identical
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.
you were right assert_eq! is much more convenient.
…ilerplate. Implemented custom packing and custom data SubPointers.
…ize. Added convienience method for u64 / u128 case.
finished subpointers for staking_log. Subpointer always read whole felt252 from memory and converts it. So I had to change approach for packing.
@moodysalem Ok. I've made final changes. Looks like task is complete. Could you do the review, please? |
#67 is new version |
Added library for Unsigned FixedPoint calculations. UFixedPoint supports basic mathematical operations.
Added new method to Staker interface:
Allows to calculate staked amount. If address staked 100 tokens for 10 seconds, that would be 1000 staked seconds. Fractions of a second are rounded down. Only full seconds count. Added additional data structure:
and in storage
and population logic into stake and withdraw methods. All searches in those logs are binary.
Closes: #61