-
Notifications
You must be signed in to change notification settings - Fork 2
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
docs: refactor technical doc #314
Conversation
bb2880a
to
e2cf454
Compare
0eaef87
to
663e9e8
Compare
e2cf454
to
cf2a59f
Compare
@andreivladbrg so after the new update, the unlock interval takes into account the last withdraw time ( I've updated the content. Without changing the numbers we can say that time values are considered since last withdraw time and not the last snapshot time. This will require updating the charts with the new labels. Alternatively, I am in favour of removing the numbers and charts and only keep the theory part in brief to understand the precision problem. This is due to the fact that with numbers and charts, the cost of maintenance of the technical doc is non-zero. The purpose of the doc is to explain delays and precision issues in calculations of withdrawn amount which imo is already well explained. What do you think? |
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 unlock interval takes into account the last withdraw time (
$wt$ ) and not snapshot time ($st$ ) anymore. Thats what the PR is about.
I don’t understand this very well. Can you please rephrase it?
As far as I can see, I think you wanted to change how we refer to the moment in time t
. Changing st
to wt
is not precisely correct. For example, these constant intervals are not correct, as the constant interval starts from st up to the moment the withdrawal happened:
Lines 393 to 395 in cf2a59f
1. $[wt, wt + 86]$ | |
2. $[wt + 87, wt + 172]$ | |
3. $[wt + 173, wt + 259]$ |
Without changing the numbers we can say that time values are considered since last withdraw time and not the last snapshot time
The number shouldn’t be changed—the delay in the current example (rps = 0.000000_011574e18
) should remain the same.
This will require updating the charts with the new labels
correct, will do
This is due to the fact that with numbers and charts, the cost of maintenance of the technical doc is non-zero. The purpose of the doc is to explain delays and precision issues in calculations of withdrawn amount which imo is already well explained.
The fact that the maintenance is non-zero is correct. I believe having concrete examples and clear graphs in our theory section will help external readers understand the delay issue more quickly and effectively. For us, it took a while to precisely understand what was actually happening, so I believe the cost is worth it to minimize future questions about the technical part.
cf2a59f
to
0513c02
Compare
@andreivladbrg do you remember what we discussed about it last week? I forgot to make notes and now can't recall what we decided about this doc. |
the most important thing we have discussed about was to we could define "withdraw previous time" and use the abbreviation Lines 388 to 389 in b01cc2d
that's what i remember |
3f37b1f
to
0513c02
Compare
c466d25
to
a9081b4
Compare
@andreivladbrg I have made the suggested changes. Let me know if it looks good now. |
@smol-ninja as discussed on slack, we should remove everything from this line and below Line 215 in 4c19888
since there is no more delay due to sd in 18 decimals, also it can be proved by these equality checks in the PR: #320 |
Not that line. MVT is one of the two problems that led us to use 18-decimal fixed-point number for rps and sd. Everything from here should be deleted. |
test: update the withdraw delay tests
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 now.
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.
lessss gooo, no more delay 🚀🚀
Refactors technical doc in line with #312.