Change price conversion function in oracle pallet to work for assets with different decimals #490
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ebma
force-pushed
the
489-change-price-conversion-function-in-oracle-pallet-to-work-for-assets-with-different-decimals
branch
from
d69ddf0 to
1e55298
Compare
ebma
force-pushed
the
489-change-price-conversion-function-in-oracle-pallet-to-work-for-assets-with-different-decimals
branch
from
1e55298 to
4fbeed1
Compare
ebma
changed the title
| CurrencyId::Stellar(asset) => asset.decimals(), | ||
| CurrencyId::XCM(index) => match index { |
There was a problem hiding this comment.
I did not check whether this is correct.
ebma
force-pushed
the
489-change-price-conversion-function-in-oracle-pallet-to-work-for-assets-with-different-decimals
branch
from
d4a2b3b to
e427fbd
Compare
gianfra-t
reviewed
Feb 16, 2024
pallets/vault-registry/src/tests.rs
Outdated
| )), | ||
| ); | ||
| assert!(max_wrapped_for_min_collateral.amount() + tolerance >= wrapped_amount); | ||
| assert!(max_wrapped_for_below_min_collateral.amount() < wrapped_amount); |
There was a problem hiding this comment.
Why don't we need the tolerance in the second assertion?
There was a problem hiding this comment.
Good question. From empiric testing I can tell you that it is not problematic and the bound holds even without the tolerance. But it would make sense to add the tolerance both ways.
There was a problem hiding this comment.
I added that tolerance also to the other check now
bogdanS98
approved these changes
Feb 19, 2024
There was a problem hiding this comment.
Great changes 👍🏼 I already got the answers to my questions from the threads created by @gianfra-t
TorstenStueber
approved these changes
Feb 20, 2024
ebma
deleted the
489-change-price-conversion-function-in-oracle-pallet-to-work-for-assets-with-different-decimals
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds changes so that the prices of assets are converted taking their respective decimals into account.
This is achieved through an extra trait
DecimalsLookup. Unfortunately, it was not possible to implement thedecimals()directly on theCurrencyIdstruct since they vary per network, ie we need to have a different implementation for Pendulum compared to Amplitude and Foucoco. The nameDecimalsLookupmight not be ideal because it implementsdecimals()andone().I decided to implement this trait for Pendulum and Amplitude in the Spacewalk repository already. This is debatable but the vault client needs to be able to have access to the decimals used on the chain it connects to which is easiest to do if we define them in this repository and re-use them in Pendulum. Otherwise, we have to duplicate the same code.
The price conversion is adopted from this function used by Nabla. I kept the functions to convert an asset to USD and vice versa around because they are nice to have in the portal since they are exposed by the RPC of our nodes.
Some tests had to be changed because they were taking simplified assumptions about the relation of price and decimals for the collateral and wrapped currency. They were mostly fixed by increasing the used numbers (because the numbers should have at least two trailing
0s in order not to face issues due to rounding) and using the price conversion in the Oracle pallet to convert between collateral and wrapped asset, taking the decimal difference into account.Closes #489.