-
Notifications
You must be signed in to change notification settings - Fork 21
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
Update yielder mechanism #221
Update yielder mechanism #221
Conversation
At this point I would like a review before working on e2e tests. I figured out something to have in mind for communication. That's because the sales are linearly interpoled between price data which could not render correctly the dynamic of the project absorption, but it is correct on data points. To fix this it is possible to loop over both absorption times and price times, however I think it is not compatible with price adaptation to compensate real vs forecasted sales. |
contract_address=contract_address, slot=slot, time=time | ||
); | ||
with_attr error_message("CarbonableFarming: the two last times must wrap absorption") { | ||
assert_not_zero(final_absorption - initial_absorption); |
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 can be "negative"
src/farming/library.cairo
Outdated
Array.replace(key=PRICE_SK, index=len - 1, value=price); | ||
|
||
// [Event] Emit event | ||
PriceUpdate.emit(time=current_time); |
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.
maybe add the updated price in the event?
} | ||
|
||
func update_last_price{syscall_ptr: felt*, pedersen_ptr: HashBuiltin*, range_check_ptr}( | ||
time: felt, price: felt |
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.
It seems the time
argument is not checked/used properly.
And isn't it also possible to update the last price with the same time? (seems the assert_not_zero
would prevent it)
src/farming/library.cairo
Outdated
|
||
// [Compute] Otherwise returns the absorption corresponding to the ratio | ||
// Keep quotient_low only since value <= project_value | ||
let (quotient_low, _, _) = uint256_mul_div_mod(value, total_absorption_u256, project_value); |
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.
Could assert quotient_high is zero
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.
Looks good to me.
I would have used Dicts for arrays, but this implem looks fine.
Should we add to the repo any python tests you made for price formula testing?
- 🐛 Implement requested changes
|
||
// [Compute] Total sale | ||
let initial_cumsale = Math.interpolate( | ||
x=time, len=len, xs=times, ys=cumsales, interpolation=LINEAR, extrapolation=CONSTANT |
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.
Format , all other Math.interpolate
variables are in column
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 cairo-format puts it like this automatically but there might be a command-line argument to tell it not to.
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 forgot to run code formatter after latest updates
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.
Minor changes but OK otw
src/farming/library.cairo
Outdated
return CarbonableFarming._compute_cumsales(); | ||
} | ||
|
||
func apr{syscall_ptr: felt*, pedersen_ptr: HashBuiltin*, range_check_ptr}(minter_address: felt) -> ( |
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.
nice trick
|
||
// [Compute] APR | ||
return ( | ||
num=(next_cumsale - current_cumsale) * YEAR_SECONDS, |
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.
💪 💦
src/yield/library.cairo
Outdated
let (decimals) = IERC20.decimals(contract_address=contract_address); | ||
let unit = Math.pow(10, decimals); | ||
with_attr error_message("CarbonableYielder: unexpected price") { | ||
assert_in_range(price, 1 * unit, 1000 * unit + 1); |
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.
Not sure but we might want to add prices equal to zero?
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.
Right, it was to ensure admin doesn't set 1 instead of 1000000 to mean 1$, but 0 should be accepted also. I'll remove the min threshold.
|
||
// [Compute] Total sale | ||
let initial_cumsale = Math.interpolate( | ||
x=time, len=len, xs=times, ys=cumsales, interpolation=LINEAR, extrapolation=CONSTANT |
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 cairo-format puts it like this automatically but there might be a command-line argument to tell it not to.
- 💄Code formatter
Closes #219