Skip to content
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

Fix token symbol testnet #2159

Merged
merged 7 commits into from
Sep 23, 2024
Merged

Fix token symbol testnet #2159

merged 7 commits into from
Sep 23, 2024

Conversation

wilwade
Copy link
Collaborator

@wilwade wilwade commented Sep 20, 2024

Goal

The goal of this PR is to fix an issue with the token symbols for testnet found while testing CheckMetadataHash extension.

Part of #2011

Discussion

  • Corrected the symbols used for node/service/src/chain_spec/frequency_paseo.rs
  • Corrected the symbols used in the substrate_wasm_builder::WasmBuilder::init_with_defaults().enable_metadata_hash
  • Added a skipped e2e test that can be run to test transacting using the metadata_hash
  • Required: Update cargo deny exclusions
  • Adjacent: Updated the API Augment packages
  • Adjacent: Updated the API Augment to have all the no-op signed extensions and remove warnings
  • Adjacent: Updated the API Augment to list all the old v1 rpcs remove warnings

Checklist

  • Updated js/api-augment for Custom RPC OR Runtime API?
  • e2e Tests added?
  • Spec version incremented?

How to Test

With the e2e test:

  • Build Frequency using the feature metadata-hash (EZ mode: edit line 109 of init.sh to have it as one of the features)
  • Set the skip in e2e/signed-extensions/checkMetadataHash.test.ts to be only
  • Run make js
  • cd e2e
  • npm i ../js/api-augment/dist/frequency-chain-api-augment-0.0.0.tgz
  • npm run test:serial
  1. See that the test passes
  2. See that there are no warnings about signed extensions
  3. See that there are no warnings about rpcs

Note: You will see an warning about PORTABLEREGISTRY: Unable to determine runtime Call type, cannot inspect sp_runtime::generic::unchecked_extrinsic::UncheckedExtrinsic However that appears to be due to an issue with the latest version of Polkadotjs/api and not related to these changes nor does it impact the running of the api.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's the new test, but it is skipped by default

@@ -175,14 +175,14 @@ export class Extrinsic<N = unknown, T extends ISubmittableResult = ISubmittableR
}

// This uses automatic nonce management by default.
public async signAndSend(inputNonce?: AutoNonce) {
public async signAndSend(inputNonce?: AutoNonce, options: Partial<SignerOptions> = {}) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Allowing the pass-through of additional options such as in this case the metadata hash

Comment on lines +65 to +72
StaleHashCheckExtension: {
extrinsic: {},
payload: {},
},
StorageWeightReclaim: {
extrinsic: {},
payload: {},
},
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are the additional signed extensions, but they don't need anything extra. This removes the warning.

@@ -10,6 +10,7 @@ use frame_support::{
PalletId,
};

// Duplicated in runtime/frequency/build.rs to keep build dependencies low
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did try importing it, but having runtime common as a build dependency was having issues and it was way too much for a few constants.

fn main() {
substrate_wasm_builder::WasmBuilder::init_with_defaults()
.enable_metadata_hash("FRQCY", 8)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here was the issue. The metadata hash was using "FRQCY" on all chains, but the chain was of course reporting different values and thus signatures were failing.

@wilwade wilwade requested review from a team, shannonwells, mattheworris, enddynayn, aramikm, claireolmstead and JoeCap08055 and removed request for a team September 20, 2024 14:03
Copy link

codecov bot commented Sep 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

@github-actions github-actions bot added the metadata-changed Metadata has changed since the latest full release label Sep 20, 2024
Copy link
Collaborator

@JoeCap08055 JoeCap08055 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Read through changes
    Looks good.

@github-actions github-actions bot added metadata-changed Metadata has changed since the latest full release and removed metadata-changed Metadata has changed since the latest full release labels Sep 20, 2024
@wilwade wilwade enabled auto-merge (squash) September 20, 2024 18:13
@@ -0,0 +1,75 @@
import { DefinitionRpc } from '@polkadot/types/types';

export const v1SubstrateRpcs: Record<string, Record<string, DefinitionRpc>> = {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this generated? If not what is the source and how it might evolve in future?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not. I generated it via the list of "unknown" rpcs. I went looking for being able to pull it from somewhere, but nothing had it from what I could find on GitHub. :(

runtime/frequency/build.rs Outdated Show resolved Hide resolved
Co-authored-by: Aramik <aramikm@gmail.com>
@github-actions github-actions bot added metadata-changed Metadata has changed since the latest full release and removed metadata-changed Metadata has changed since the latest full release labels Sep 23, 2024
Copy link
Collaborator

@mattheworris mattheworris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Read through the changes
  • Confirmed the testing results
    🚢 it!

Copy link
Collaborator

@aramikm aramikm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Read the code. Looks good!

@wilwade wilwade merged commit a49d337 into main Sep 23, 2024
27 checks passed
@wilwade wilwade deleted the bug/fix-token-symbol-testnet branch September 23, 2024 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
metadata-changed Metadata has changed since the latest full release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants