Skip to content
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 staker v2 no fp and subpointers #67

Open
wants to merge 37 commits into
base: main
Choose a base branch
from

Conversation

baitcode
Copy link

Same as in staker v2 but way less code

…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
…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.
inline everything remove storage read optimisations
src/staker.cairo Outdated
@@ -78,6 +86,7 @@ pub mod Staker {
const TWO_POW_64: u128 = 0x10000000000000000;
const TWO_POW_192: u256 = 0x1000000000000000000000000000000000000000000000000;
const TWO_POW_192_DIVISOR: NonZero<u256> = 0x1000000000000000000000000000000000000000000000000;
const HALF: u128 = 0x80000000000000000000000000000000_u128;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TWO_POW_127 is more descriptive

src/staker.cairo Outdated
@@ -253,6 +264,12 @@ pub mod Staker {
self
.amount_delegated
.write(delegate, self.insert_snapshot(delegate, get_block_timestamp()) + amount);

let total_staked = self.total_staked.read();
assert(total_staked + amount >= total_staked, 'BAD AMOUNT');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this supposed to be an overflow check? this cannot happen ever because addition is checked by default

Comment on lines 445 to 446
assert_eq!(value.high, integer);
assert_eq!(value.low, fractional);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

think it would be more readable if you just wrote assert_eq!(value, u256 {high: integer, low:fractional}) and also inlined that into the below test

pub(crate) timestamp: u64,

// Only 128+32=160 bits are used
// TODO: add validation checks
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wdym?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh. I've already did that. Forgot to remove. Thank you for catching.

const TWO_POW_32: u64 = 0x100000000_u64;
const MASK_32_BITS: u128 = 0x100000000_u128 - 1;
const TWO_POW_160: u256 = 0x10000000000000000000000000000000000000000;
pub const MAX_FP: u128 = 0x8000000000000110000000000000000_u128;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is this value?

Copy link
Author

@baitcode baitcode Jan 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maximum value that can be store in felt252 is MAX_V = 0x800000000000011000000000000000000000000000000000000000000000000.
MAX_FP = MAX_V >> 128 (maximum value of highest 123 bit of felt252).

src/staker.cairo Outdated
}
};

let seconds_diff = (timestamp - log_record.timestamp) / 1000;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

block timestamps are in seconds arent they? why are we dividing by 1k

Copy link
Author

@baitcode baitcode Jan 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is absolutely right. I can't believe tests did not catch it. Aaaa found it. Turned out I saved ms in record. Thank you for catching this. Need to add think on how to test storage.

* uniform get_block_timestamp usage
* store log_record timestamp in seconds
* inline test assert for fp value
* rename HALF to TWO_POW_127
src/staker.cairo Outdated
fn get_cumulative_seconds_per_total_staked_at(
self: @ContractState, timestamp: u64,
) -> u256 {
let timestamp_seconds = timestamp / 1000;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

timestamp should not be expressed in milliseconds when everything onchain is in seconds

because of this division, there is no difference between e.g. 4000 and 4999

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh my! I though on-chain everything is in ms.

src/staker.cairo Outdated
// substract fixed point values
let divisor = next_log_record.cumulative_seconds_per_total_staked
- log_record.cumulative_seconds_per_total_staked;
assert(divisor.high < MAX_FP, 'FP_OVERFLOW');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can this actually happen?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, in my practice things tend to happen. Assertions are also a good way of documenting code. But I see your point. Will remove.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants