pendulum-chain / spacewalk

Apache License 2.0
34 stars 7 forks source link

503 make spacewalk benchmarks run successfully for pendulum runtimes #505

Closed bogdanS98 closed 4 months ago

bogdanS98 commented 5 months ago

Closes #503.

Fixes included in this PR:

Also benchmarks were re-ran after adding the fixes mentioned above. (see commit)

bogdanS98 commented 5 months ago

Nice, great changes 🙏

I just thought that we might want to remove this function to avoid any confusion. Otherwise we have some very similar looking functions and we might forget about it. It's probably best if we use the new get_wrapped_currency_id<T>() function everywhere. To make it available also in the other tests and not just runtime benchmarks, we could change the macro condition to:

#[cfg(any(feature = "runtime-benchmarks", feature = "testing-constants))]

That's just one idea, maybe you have a better one @bogdanS98.

I agree that we should remove this function since it's confusing and not really necessary. I think using get_wrapped_currency_id<T>() in tests is a good idea, but we can also directly use the constants in testing_constants. WDYT @ebma?

ebma commented 5 months ago

I think it's fine to replace it with the constant. Depends on how often we use it but should be fine.

ebma commented 5 months ago

We still need to remove the function itself, please do that @bogdanS98 😬

ebma commented 5 months ago

@bogdanS98 We still have this 'no space left on device' error but for some reason it only happens on this branch :( any ideas?

bogdanS98 commented 5 months ago

@bogdanS98 We still have this 'no space left on device' error but for some reason it only happens on this branch :( any ideas?

Looking into this, I think I'm going to use the same storage cleanup step as in this workflow.

ebma commented 5 months ago

Hmm yes, let's give that a shot 🤞

bogdanS98 commented 5 months ago

The code looks good 👍!

To get the new spacewalk benchmarks, in the pendulum repo it was enough to modify the rev to point to the latest commit of this branch, added type GetWrappedCurrencyId = GetWrappedCurrencyId; to the runtimes and compile ?

I was trying to do the same and get an error, but I am probably missing some spacewalk dependency to compile so I wanted to confirm if I understand the process.

Yes, that's all you need. What error do you get?

gianfra-t commented 5 months ago

Particularly I get the error

 get_relay_chain_currency_id as get_collateral_currency_id, get_wrapped_currency_id, *,
  |                                                                ^^^^^^^^^^^^^^^^^^^^^^^
  |                                                                |
  |                                                                no `get_wrapped_currency_id` in `getters`
  |                                                                help: a similar name exists in the module: `get_native_currency_id`

Which probably points to an incorrect spacewalk dependency replacement. I just replaced 3c03e759a6f600ea72d636d4f9cc4b05d633201c with 8699f80e50aeca63485a25119e06cb6123b05d01, maybe I am missing a spacewalk import with a different revision?

bogdanS98 commented 5 months ago

Particularly I get the error

 get_relay_chain_currency_id as get_collateral_currency_id, get_wrapped_currency_id, *,
  |                                                                ^^^^^^^^^^^^^^^^^^^^^^^
  |                                                                |
  |                                                                no `get_wrapped_currency_id` in `getters`
  |                                                                help: a similar name exists in the module: `get_native_currency_id`

Which probably points to an incorrect spacewalk dependency replacement. I just replaced 3c03e759a6f600ea72d636d4f9cc4b05d633201c with 8699f80e50aeca63485a25119e06cb6123b05d01, maybe I am missing a spacewalk import with a different revision?

I think you better merge this branch into yours for testing.