-
Notifications
You must be signed in to change notification settings - Fork 16
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
Support for replicating tokens on chain #311
Conversation
This comment has been minimized.
This comment has been minimized.
0704155
to
1923412
Compare
This comment has been minimized.
This comment has been minimized.
|
OK, I think this one is finally ready for review @dorranh! The description is also now up to date. After merging this I think we'll be at the point where the README should have separate build/deployment/testing instructions per configuration. Hopefully the API won't really change after this PR so we can start looking at #118 soon after. I had to deactivate three tests that are configuration-specific for CI to pass, but we should be able to renable those tests once #284 is addressed (partially or completely; I might be able to fix that rather easily, being on the OCaml side). Either way, I'd go ahead and merge this PR without them for 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.
Overall looks good to me! I just had one question regarding units of measure. If the comment is not actually an issue then I am happy merging 🙂
(** Calculate the current target based on the current quantity, and the current | ||
price of kit in ctok (as given by the CFMM). | ||
{[ | ||
target_{i+1} = FLOOR (q_{i+1} / kit_in_ctok_{i+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.
Is the target still dimensionless if we've removed index
from this equation? To me this reads: [1/kit] / [ctok/kit] => [1/ctok]
. Perhaps q has different units in the case of a token-based checker? Not sure if this has implications for the system though.
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 just re-read the new document covering the units of measure and am still a bit confused about which units q
is supposed to have. Perhaps we could discuss this offline as well.
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.
Yeah, let's discuss this more offline. For reference, here's how I've been thinking about the two cases:
- index-tracking CHF
index : tok/CHF p_cfmm : tok/kit => (index/p_cfmm) : kit/CHF
- token-tracking tzCHF
p_cfmm : tzCHF/kit => (1/p_cfmm) : kit/tzCHF
let price = fixedpoint_of_ratio_floor (make_ratio (Ligo.int idx) tok_scaling_factor_int) in | ||
let price = | ||
let (num, den) = oracle_price in | ||
fixedpoint_of_ratio_floor (make_ratio (Ligo.int num) (Ligo.int den)) in |
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 we should add a comment here to make it extra obvious that we are losing some precision?
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.
Ah yes, I added a small note in a9d6a88 👍
|
@dorranh shall I go ahead and merge this? |
@gkaracha, yep I think it's good to go following our separate discussion of the |
Closes #263.
%getCfmmPrice
(as opposed to%getPrice
which is the one we use for the mock oracle). This is not very descriptive and does not indicate the units we expect the fraction to have (tok/ctok), but it's only a placeholder for now. We might want to change this in the future, or even perhaps get it from the yaml file at build time.Also: currently both mock oracles (index and cfmm) have an
update
entrypoint but since this is not part of Checker's spec I saw no good reason to deviate on the name of this one too.tracking_type
) which can take one of two values:index
ortoken
.nat
s (CHF in TEZ, (TEZ/CHF)). This reduces the number of differences between token-based and index-based instances quite some.docs/prices.md
with some notes about price calculations, taken from Generalize checker to be able to replicate tokens on chain #263 (comment).checker-index-tez.yaml
,checker-index-fa2.yaml
, andchecker-token-fa2.yaml
.