fix: bug on oracle fetching #1750

mustermeiszer · 2024-02-29T19:50:21Z

Description

The current oracle does check local representation of wrongly.

Changes and Descriptions

Fix checking local representation of local to variant, instead of variant to local
Add integration tests for the OracleRatioProvider

Checklist:

I have added Rust doc comments to structs, enums, traits and functions
I have made corresponding changes to the documentation
I have performed a self-review of my code
I have added tests that prove my fix is effective or that my feature works

mustermeiszer · 2024-02-29T19:51:01Z

@wischli this must go in before #1749

mustermeiszer · 2024-02-29T22:13:36Z

runtime/integration-tests/src/generic/cases/oracles.rs

+			assert_eq!(
+				orml_asset_registry::Metadata::<T>::get(LocalUSDC.id()),
+				None
+			);
+			assert_eq!(
+				orml_asset_registry::Metadata::<T>::get(DomainUSDC.id()),
+				None
+			);


@wischli @lemunozm these assertions are triggered. Why on earth should this storage be populated already? I either am looking at the reason without seeing it, or something is really off here.

Could this be due to the migration being performed before initializing the tests? Our migration populates the state with those currencies IIRC

Oh man. That could be. Haha. But then it is not working as intended. As the local representation is missing on asset 100_001. I will try to use different ids.

Thanks @lemunozm!

But here we are using id 100_0001 instead of 100_001 LpEthUsdc. I suppose the local asset id of 1 is the issue then. Compiling right now with my cherry-pick for disabling migrations in std: 0e53752

Yup, works now! Eagle eye @lemunozm 🎯

…esentation-bug

lemunozm

LGTM. I think the currencies testing API now is more versatile.

Maybe we could extend the integration-tests with some pallet-token-mux call to ensure the whole use case is working well

lemunozm · 2024-03-01T06:53:04Z

runtime/integration-tests/src/generic/utils/currency.rs

+			transferability: CrossChainTransferability::Xcm(XcmMetadata {
+				fee_per_second: Some(1_000),
+			}),


Just wondering why Usd6 has this property but Usd12 and Usd18 do not. Could we use the same for all of them?

I just copy pasted everything. Not sure, why it was there.

Seems like we want to have default transferability here instead?

lemunozm · 2024-03-01T06:54:20Z

runtime/integration-tests/src/generic/utils/currency.rs

+		10u128.pow(self.decimals())
+	}
+
+	fn symbol(&self) -> &'static str;


Most of the time, we do not care about the symbol for testing. Could we use a default method here with some random/empty string? Maybe "Unnamed" or so.

mustermeiszer · 2024-03-01T08:51:13Z

Maybe we could extend the integration-tests with some pallet-token-mux call to ensure the whole use case is working well

I agree that this would be useful. But I do not have time for that. I am testing everything in demo at the moment. Thats also where I found this issue. I think the best would be to merge #1720 and then write integration test verifying the whole flow...

lemunozm

Just want to change one thing that I think it's important to modify/clarify.

Everything else looks great!

lemunozm · 2024-03-01T09:38:54Z

runtime/altair/src/migrations.rs

+#[cfg(feature = "std")]
+pub type UpgradeAltair1034 = ();
+


Maybe there exists a way to tune RuntimeEnv to avoid this issue from the tests? I do not like adding this addition to the runtime too much with the aim of avoiding migrations in testing. Future readers of this will not know why this is so, and we can be subject to unexpected behavior in the future. I would add a comment at least explain why this

Another option is tweaking the block creation in RuntimeEnv somehow to skip the migration. Not sure if this is something easy to do

Since this is my forced change which should not have been pushed to this branch in the first place, I will just revert it. However, it improves integration test time because it blocks migrations for each node we spawn.

What unexpected behaviour are you fearing here @lemunozm? I think it is unexpected that migrations run for integration tests which is fixed this way. It's not the nicest solution but it does the job quickly without imposing any risk.

Fixed in 4f41f24

However, it improves integration test time because it blocks migrations for each node we spawn.

Good point!

What unexpected behaviour are you fearing here @lemunozm?

Could some uses of the node underlying it exist that compile the runtime as std and where do we expect some kind of migration, maybe some migration testing or so? Probably not now but it scared me 🙏🏻

Fixed in 4f41f24

Love the solution! Maybe this can even be ported to fudge to avoid migrations there too (not mandatory for this PR)

wischli

The Usd6 still has XCM transferability enabled. Apart from that LGTM!

wischli · 2024-03-01T10:08:33Z

runtime/integration-tests/src/generic/envs/runtime_env.rs

+		// NOTE: Setting the current on-chain runtime version to the latest one, to
+		//       prevent running migrations
+		parachain_ext.execute_with(|| {
+			frame_system::LastRuntimeUpgrade::<T>::put(LastRuntimeUpgradeInfo::from(
+				<T as frame_system::Config>::Version::get(),
+			))
+		});


Awesome change, thanks for that!

wischli · 2024-03-01T10:09:46Z

runtime/integration-tests/src/generic/utils/currency.rs

+			transferability: CrossChainTransferability::Xcm(XcmMetadata {
+				fee_per_second: Some(1_000),
+			}),


Seems like we want to have default transferability here instead?

mustermeiszer · 2024-03-01T10:15:01Z

The Usd6 still has XCM transferability enabled. Apart from that LGTM!

centrifuge-chain/runtime/integration-tests/src/generic/utils/currency.rs

Lines 74 to 76 in 10aed20

    
           transferability: CrossChainTransferability::Xcm(XcmMetadata { 
        
           	fee_per_second: Some(1_000), 
        
           }),

That is comming from main, so I am not keen on changing that here. @lemunozm your commit apparently.

lemunozm · 2024-03-01T11:22:01Z

TBH, I do not remember. Maybe a copy-paste from another place. I would put there what you think is cleaner/defaulted/simpler (probably what @wischli said)

lemunozm · 2024-03-01T13:02:25Z

So we know now the reason 😆, we needed it for the proxy cross-change tests.

I think this is a change added by @cdamian (who could explain the whys better) in the PR I opened about proxies, that's why git blame tell about me. Maybe we can create a currency in our own proxy tests for cross-chain things with the required metadata, using the new way of doing it. WDYT?

mustermeiszer · 2024-03-01T13:53:54Z

@lemunozm fixed. Can you approve if you are happy?

mustermeiszer · 2024-03-01T15:41:12Z

Will force merge as @wischli approved and @lemunozm is off the next week. I anyways addressed all issues from Luis.

lemunozm · 2024-03-01T16:44:55Z

Sorry for my late review!

Really cool the new register_currency() method 💯

fix: bug on oracle fetching

1cf112d

mustermeiszer requested a review from wischli February 29, 2024 19:50

feat: tests for oracle provider. WTF...

b1734f4

mustermeiszer commented Feb 29, 2024

View reviewed changes

mustermeiszer added 2 commits February 29, 2024 23:17

feat: enable for runtimes

0da86c3

Merge remote-tracking branch 'origin/main' into fix/oracle-local-repr…

5043048

…esentation-bug

lemunozm reviewed Mar 1, 2024

View reviewed changes

wischli added 2 commits March 1, 2024 08:48

chore: add no-std feat gate migration tuples

4c2eb84

fix: compilation

c1530b5

wischli assigned mustermeiszer Mar 1, 2024

wischli added the I2-bug The code fails to follow expected behaviour. label Mar 1, 2024

wischli added this to the Centrifuge 1025 milestone Mar 1, 2024

mustermeiszer added 2 commits March 1, 2024 09:47

feat: final tests

9e34107

chore: rm unused test mod

39a672d

mustermeiszer added the D0-ready Pull request can be merged without special precaution and notification. label Mar 1, 2024

mustermeiszer enabled auto-merge (squash) March 1, 2024 09:26

lemunozm requested changes Mar 1, 2024

View reviewed changes

fix: change request

4f41f24

wischli previously approved these changes Mar 1, 2024

View reviewed changes

fix: review request

edccc3b

mustermeiszer dismissed wischli’s stale review via edccc3b March 1, 2024 11:53

fix: unused import

d37f122

fix: integration tests

497d704

mustermeiszer disabled auto-merge March 1, 2024 15:41

mustermeiszer merged commit 472d1af into main Mar 1, 2024
9 checks passed

wischli deleted the fix/oracle-local-representation-bug branch March 1, 2024 16:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: bug on oracle fetching #1750

fix: bug on oracle fetching #1750

mustermeiszer commented Feb 29, 2024 •

edited

Loading

mustermeiszer commented Feb 29, 2024

mustermeiszer Feb 29, 2024

lemunozm Mar 1, 2024 •

edited

Loading

mustermeiszer Mar 1, 2024

wischli Mar 1, 2024 •

edited

Loading

wischli Mar 1, 2024

lemunozm left a comment

lemunozm Mar 1, 2024

mustermeiszer Mar 1, 2024

wischli Mar 1, 2024

lemunozm Mar 1, 2024

mustermeiszer Mar 1, 2024

mustermeiszer commented Mar 1, 2024

lemunozm left a comment •

edited

Loading

lemunozm Mar 1, 2024

lemunozm Mar 1, 2024

wischli Mar 1, 2024

mustermeiszer Mar 1, 2024 •

edited

Loading

lemunozm Mar 1, 2024

lemunozm Mar 1, 2024

wischli left a comment

wischli Mar 1, 2024

wischli Mar 1, 2024

mustermeiszer commented Mar 1, 2024

lemunozm commented Mar 1, 2024 •

edited

Loading

lemunozm commented Mar 1, 2024

mustermeiszer commented Mar 1, 2024

mustermeiszer commented Mar 1, 2024

lemunozm commented Mar 1, 2024

fix: bug on oracle fetching #1750

fix: bug on oracle fetching #1750

Conversation

mustermeiszer commented Feb 29, 2024 • edited Loading

Description

Changes and Descriptions

Checklist:

mustermeiszer commented Feb 29, 2024

Choose a reason for hiding this comment

lemunozm Mar 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wischli Mar 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lemunozm left a 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

mustermeiszer commented Mar 1, 2024

lemunozm 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

mustermeiszer Mar 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wischli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mustermeiszer commented Mar 1, 2024

lemunozm commented Mar 1, 2024 • edited Loading

lemunozm commented Mar 1, 2024

mustermeiszer commented Mar 1, 2024

mustermeiszer commented Mar 1, 2024

lemunozm commented Mar 1, 2024

mustermeiszer commented Feb 29, 2024 •

edited

Loading

lemunozm Mar 1, 2024 •

edited

Loading

wischli Mar 1, 2024 •

edited

Loading

lemunozm left a comment •

edited

Loading

mustermeiszer Mar 1, 2024 •

edited

Loading

lemunozm commented Mar 1, 2024 •

edited

Loading