Support all fractional places #1049

sea212 · 2023-07-21T16:29:37Z

What does it do?

The PR adjusts all foreign token balance representations to the base BASE, currently 10^10.

The PR solves the problem of aligning the decimal fractional places of all foreign tokens with the native representation by modifying the TransactAsset implementation that is used in the xcm-executor. Until now the MultiCurrencyAdapter has been used to as an implementation of the TransactAsset. This PR adds a generic wrapper AlignedFractionalTransactAsset around a TransactAsset implementation, that applies proper alignment of token balances before calling the wrapped implementation (in that case MultiCurrencyAdapter.

The TransactAsset does determine how tokens are deposited to an account after withdrawing them from the holding register, how they are withdrawn from an account before being deposited in the holding register and how they are transfered. Additional functions to handle teleports for example exist, but as of now the currently used MultiCurrencyAdapter does not support teleports (see this) and Zeitgeist does not utilize teleports.

Tests ensure proper functioning in any case (ZTG out and in, token with more than 10 fractional decimals out and in, token with less than 10 fractional decimals out and in).

What important points should reviewers know?

XCM messages always use the canonical representation of token balances. For example, when 1 GLMR (Moonbeam) would be sent to Zeitgeist, it would be denoted as 1_000_000_000_000_000_000 (10^18) in the XCM message. Locally, it would be aligned to BASE, which (assuming there are no fees) results in 10_000_000_000 (10^10). When crafting an XCM to send 1 GLMR from Zeitgeist to Moonbeam, the XCM must contain the 1_000_000_000_000_000_000 (10^18), even though locally it is represented as 10_000_000_000 (10^10). The conversion can be done by receiving the decimals from the asset's metadata within the AssetRegistry. The rationale behind this is that the XCM should have the same meaning everywhere, as other projects also execute XCM on Zeitgeist (for example to transfer ZTG to their protocol). Consequently, using BASE representation in XCM for any token and aligning it afterwards is not a coherent solution.
The interpretation of fee_factor of assets metadata within the AssetRegistry has been changed to assume fee_factor being in the native base BASE. From now on, the existential deposit and fee factor are assumed to be noted in BASE.
No migrations are necessary as no tokens with a base other than BASE exist on Zeitgeist mainnet as of now. On Battery Station (Rococo) testnet, Rococo is already onboarded (12 decimal places). Alignment will happen manually in that case.

Is there something left for follow-up PRs?

Add teleport support in AlignedFractionalTransactAsset #1055

What alternative implementations were considered?

Forking and adjusting the xcm-executor and xtokens
Modifying XCM to always use BASE representation for token amounts

Are there relevant PRs or issues?

closes #969

References

Proposal: Support for tokens with arbitrary fractional decimal places

codecov-commenter · 2023-07-29T17:57:02Z

Codecov Report

Merging #1049 (c29b49c) into main (bac24e6) will increase coverage by 0.00%.
Report is 13 commits behind head on main.
The diff coverage is 89.69%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@           Coverage Diff           @@
##             main    #1049   +/-   ##
=======================================
  Coverage   92.93%   92.94%           
=======================================
  Files          92       92           
  Lines       21589    21581    -8     
=======================================
- Hits        20064    20058    -6     
+ Misses       1525     1523    -2

Flag	Coverage Δ
tests	`92.94% <89.69%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
zrml/court/src/weights.rs	`0.00% <0.00%> (ø)`
zrml/liquidity-mining/src/weights.rs	`0.00% <0.00%> (ø)`
zrml/styx/src/weights.rs	`0.00% <0.00%> (ø)`
zrml/swaps/src/weights.rs	`84.81% <89.06%> (ø)`
zrml/prediction-markets/src/weights.rs	`94.01% <92.63%> (+0.73%)`	⬆️
zrml/authorized/src/weights.rs	`100.00% <100.00%> (ø)`
zrml/global-disputes/src/weights.rs	`100.00% <100.00%> (ø)`

... and 1 file with indirect coverage changes

Chralt98

It is an interesting approach to handle foreign assets easier on our chain. Especially that you calculated the risk of falling precision for having more foreign decimals places than our native base of ten.

Chralt98 · 2023-07-31T09:16:08Z

runtime/zeitgeist/src/xcm_config/config.rs

+            }
+        }
+
+        asset.clone()


I see. No modification of decimal places is also allowed, because it's put into the UnkownAssetRegistry. Assume the following scenario: An asset arrives which we don't have registered yet. So it arrives in the UnknownAssetRegistry. Now we know which asset it is and register it in the AssetRegistry. Is the asset amount updated to 10 decimal places too in this case?

Chralt98 · 2023-07-31T10:14:43Z

runtime/zeitgeist/src/xcm_config/config.rs

+                        let power = decimals.saturating_sub(native_decimals);
+                        let adjust_factor = 10u128.saturating_pow(power);
+                        // Floors the adjusted token amount, thus no tokens are generated
+                        Fungible(amount.saturating_div(adjust_factor))


This seems to regard the ingoing asset to Zeitgeist since the high number of decimals need to decrease (saturating_div). So this seems totally reasonable for the deposit to Zeitgeist case.

But where is the withdraw case? So the outgoing asset from Zeitgeist for the higher decimal case?

Any XCM uses the global canonical representation, i.e. 18 fractional decimal places for ETH.

Simplified:

deposit: When a reserve asset transfer with ETH is incoming, it puts the unadjusted ETH in the holding register. Then it deposits the adjusted ETH using the adjusted deposit function into the target pallet and account. Then it subtracts the unadjusted value from the holding register.

withdraw: When a reserve asset transfer with ETH is sent, it withdraws the adjusted ETH using the adjusted withdraw function from the target pallet and acount and afterwards puts the unadjusted ETH in the holding register (since the XCM contains the global canonical representation, in this case 18 fractional decimal places).

deposit: XCM in (unadjusted ETH) -> holding register (unadjusted) -> tokens (deposit, adjusted) -> update holding register (unadjusted)
withdraw: XCM out (unadjusted ETH) -> tokens (withdraw, adjusted) -> holding register (unadjusted)

The trick here is that we demand the global canonical representation for any token. Simplified, if 1 ETH enters, the XCM contains the amount 10^18. If an XCM is created on Zeitgeist to send ETH, the XCM contains the amount 10^18 as value, although internally it is stored as 10^10.

I see. So the point that I missed is that any XCM message either withdraw or deposit is (hopefully) always constructed with the external decimal representation (not our internal ten base representation).

I also missed the point in general that your AlignedFractionalTransactAsset is only meant for the internal representation to get data from the XCM into the tokens pallet. AlignedFractionalTransactAsset is not meant to get the asset amount out to the external representation, since the external representation is always the value inside the XCM.

It reminds me about a one-way hash function into the internal representation only.

runtime/zeitgeist/src/xcm_config/config.rs

Chralt98 · 2023-07-31T10:21:48Z

runtime/zeitgeist/src/xcm_config/config.rs

+        from: &MultiLocation,
+        to: &MultiLocation,
+    ) -> Result<Assets, XcmError> {
+        let asset_adjusted = Self::adjust_fractional_places(asset);


I am thinking about what transfer_asset actually does for the orml MultiCurrencyAdapter. How is Zeitgeist involved here, since the from could be Moonbeam and to could be Astar? Is it outgoing or ingoing for Zeitgeist? So, did we already adjust the decimal places internally and then call transfer_asset which transfers the amount to a completely different chain? If that is the case, then we withdraw from Zeitgeist, but need to recover the original decimal places. If it is a deposit to Zeitgeist, then we need to adjust to the new decimal places (base 10).

TransferAsset is used to transfer an asset on a remote destination from the sending account to a receiving account on the same chain, like in this example (sent from sending account into parachain account). Messages that contain a location outside of Zeitgeist will fail in execution.

Ok, I think I see now. Means other parachains can remotely transfer assets between two Zeitgeist accounts.

runtime/zeitgeist/src/xcm_config/fees.rs

runtime/battery-station/src/xcm_config/fees.rs

runtime/zeitgeist/src/integration_tests/xcm/tests/transfers.rs

Co-authored-by: Chralt <[email protected]>

maltekliemann · 2023-08-02T16:29:21Z

docs/changelog_for_devs.md

+- ⚠️ All tokens now use 10 fractional decimal places.
+  - cross-consensus messages (XCM) assume the global canonical representation for token balances.
+- The token metadata in the asset registry now assumes that the existential deposit and fee factor
+  are stored in base 10,000,000,000.


This should be reformatted using prettier.

The alignment of line 22 is intentional, it is a sub point of the one in line 21.

sea212 added 2 commits July 21, 2023 13:27

Create AlignedFractionalTransactAsset struct

7139001

Implement adjusted deposit_asset

aa56d29

sea212 added the s:in-progress The pull requests is currently being worked on label Jul 21, 2023

sea212 added this to the v0.3.11 milestone Jul 21, 2023

sea212 self-assigned this Jul 21, 2023

sea212 added 10 commits July 21, 2023 18:52

Make compileable

1b3d944

Allign fractional decimal places on Battery Station

e8afa4c

Partially implement tests on Battery Station

f299eb7

User proper adjusted transfer values in tests

d24e39a

Finalize tests on Battery Station

fae3d54

Align fractional decimal places on Zeitgeist

9eae276

Add btc test to Zeitgeist

b79847f

Extended Zeitgeist XCM test with ETH (18 decimals)

a0f9f0f

Cargo fmt

9852c83

Use BASE representation in token metadata

ddc11bf

sea212 added s:review-needed The pull request requires reviews and removed s:in-progress The pull requests is currently being worked on labels Jul 29, 2023

sea212 added s:in-progress The pull requests is currently being worked on and removed s:review-needed The pull request requires reviews labels Jul 30, 2023

sea212 added 3 commits July 30, 2023 10:40

Align decimal places for transfers

31ce472

Merge branch 'main' into sea212-support-all-fractional-places

354f7c4

Update changelog + format

0858154

sea212 added s:review-needed The pull request requires reviews and removed s:in-progress The pull requests is currently being worked on labels Jul 30, 2023

sea212 requested a review from Chralt98 July 30, 2023 08:49

sea212 added s:in-progress The pull requests is currently being worked on and removed s:review-needed The pull request requires reviews labels Jul 30, 2023

Satisfy Clippy

6326d6f

sea212 removed the s:in-progress The pull requests is currently being worked on label Jul 30, 2023

sea212 added the s:review-needed The pull request requires reviews label Jul 30, 2023

Chralt98 reviewed Jul 31, 2023

View reviewed changes

sea212 added s:in-progress The pull requests is currently being worked on and removed s:review-needed The pull request requires reviews labels Jul 31, 2023

sea212 and others added 3 commits July 31, 2023 14:29

Update runtime/zeitgeist/src/xcm_config/fees.rs

7fca3c1

Co-authored-by: Chralt <[email protected]>

Update runtime/zeitgeist/src/xcm_config/fees.rs

218bd90

Co-authored-by: Chralt <[email protected]>

Apply suggestions from code review

c29b49c

Co-authored-by: Chralt <[email protected]>

Chralt98 approved these changes Aug 2, 2023

View reviewed changes

sea212 added s:review-needed The pull request requires reviews and removed s:in-progress The pull requests is currently being worked on labels Aug 2, 2023

Chralt98 requested a review from maltekliemann August 2, 2023 16:49

maltekliemann approved these changes Aug 2, 2023

View reviewed changes

sea212 added s:accepted This pull request is ready for merge and removed s:review-needed The pull request requires reviews labels Aug 3, 2023

sea212 merged commit 41a2564 into main Aug 3, 2023
21 checks passed

sea212 deleted the sea212-support-all-fractional-places branch August 3, 2023 09:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support all fractional places #1049

Support all fractional places #1049

sea212 commented Jul 21, 2023 •

edited

Loading

codecov-commenter commented Jul 29, 2023 •

edited

Loading

Chralt98 left a comment •

edited

Loading

Chralt98 Jul 31, 2023

Chralt98 Jul 31, 2023

Chralt98 Jul 31, 2023

sea212 Jul 31, 2023

sea212 Jul 31, 2023

Chralt98 Aug 2, 2023

Chralt98 Jul 31, 2023

sea212 Jul 31, 2023

Chralt98 Aug 2, 2023 •

edited

Loading

maltekliemann Aug 2, 2023

sea212 Aug 3, 2023

Support all fractional places #1049

Support all fractional places #1049

Conversation

sea212 commented Jul 21, 2023 • edited Loading

What does it do?

What important points should reviewers know?

Is there something left for follow-up PRs?

What alternative implementations were considered?

Are there relevant PRs or issues?

References

codecov-commenter commented Jul 29, 2023 • edited Loading

Codecov Report

Chralt98 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Chralt98 Aug 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sea212 commented Jul 21, 2023 •

edited

Loading

codecov-commenter commented Jul 29, 2023 •

edited

Loading

Chralt98 left a comment •

edited

Loading

Chralt98 Aug 2, 2023 •

edited

Loading