-
Notifications
You must be signed in to change notification settings - Fork 51
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
Onboarding: Add Issuance settings screen #524
Conversation
@PJColombo is attempting to deploy a commit to the 1hive Team on Vercel. A member of the Team first needs to authorize it. |
b0af210
to
b6a84e6
Compare
…and plot the chart
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/1hive/honey-pot/6iBo2FK83GVhXCkwFxkN7dJMmEoj |
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.
Looking good 🙌 I merge with the garden
branch to reduce the review files.
Left a few comments related to the calculation formula.
@@ -1,3 +1,4 @@ | |||
export const MINUTE_IN_SECONDS = 60 | |||
export const HOUR_IN_SECONDS = MINUTE_IN_SECONDS * 60 | |||
export const DAY_IN_SECONDS = HOUR_IN_SECONDS * 24 | |||
export const YEARS_IN_SECONDS = 365 * DAY_IN_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.
I wonder if there are not issues with considering years as always 365 days long? I remember that solidity removed the years
in one of the latest versions because the above is not always true.
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! The parameter is defined as maxAdjustmentPerSecond
and we are using this same number in the Issaunce tests in order have this parameter annualized. However, as I posted in this section of my post on the dynamic issuance v1, this annualization is misleading, so I'm changing the parameter name to Throttle in the frontend, and I use another parameter in the Issuance v2: Recovery Time.
src/components/Onboarding/Screens/Apps/IssuanceSettings/IssuanceChart.js
Show resolved
Hide resolved
let supply = 1 | ||
const ratio = [balance / supply] | ||
for (let i = 0; i < 365; i++) { | ||
const adj = ((1.0 - balance / supply / targetRatio) / 365) * supply |
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.
What do you think of adding the extra parameters to improve readability and help to understand the math involved?
src/components/Onboarding/Screens/Apps/IssuanceSettings/IssuanceChart.js
Show resolved
Hide resolved
@0xGabi I added comments in the code so it's now easier to follow. Thanks for pointing it out! |
This PR implements the issuance settings screen.
It also includes the issuance chart but we still need to generate the data for it. @sembrestels was working on the model. However, the calculations are not entirely correct and they still need to be validated.