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

Da 179/upgrade 5.0.0 #742

Merged
merged 58 commits into from
May 30, 2022
Merged

Da 179/upgrade 5.0.0 #742

merged 58 commits into from
May 30, 2022

Conversation

polymath-eric
Copy link
Collaborator

Description

Upgrades the SDK for the 5.0.0 chain. The chain metatdata has made some changes. Notably the chain types are prefixed with PolymeshPrimitive or PalletSomePallet. While this makes the code more verbose, it does eliminate a lot of snake case in the Codecs which cleans up the eslint disable comments. Overall I think it reads cleaner, and the long names don't bother me that much besides the ones the stutter (e.g. Compliance Manager ones)

Breaking Changes

I think the external API hasn't changed, but would need to double check to be sure. For the most part the breaking changes are with the interaction with polkadot.js

JIRA Link

https://polymath.atlassian.net/browse/DA-260
https://polymath.atlassian.net/browse/DA-179

Checklist

  • Updated the Readme.md (if required) ?

@polymath-eric polymath-eric changed the base branch from master to alpha April 29, 2022 02:05
@polymath-eric polymath-eric marked this pull request as draft April 29, 2022 02:07
Copy link
Collaborator Author

@polymath-eric polymath-eric left a comment

Choose a reason for hiding this comment

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

I found this to be a pretty challenging upgrade. I had to fill in some holes in my knowledge with how polkadot schemas work.

Test coverage isn't perfect, and I think some conversion and mock functions need to be touched up in their params, but overall this seems to be working.

I'll pick this up when I am back Thursday, but if someone else picks it up and has questions feel free to Slack me.

"generate:defs": "ts-node --skip-project node_modules/.bin/polkadot-types-from-defs --package polymesh-types --input ./src/polkadot",
"generate:meta": "ts-node --skip-project node_modules/.bin/polkadot-types-from-chain --package polymesh-types --endpoint ws://localhost:9944 --output ./src/polkadot --strict",
"generate:defs": "ts-node --skip-project node_modules/.bin/polkadot-types-from-defs --endpoint metadata.json --package polymesh-types --input ./src/polkadot/",
"generate:meta": "ts-node --skip-project node_modules/.bin/polkadot-types-from-chain --endpoint metadata.json --package polymesh-types --output ./src/polkadot --strict",
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 seemed like one of these commands didn't like the ws endpoint with the chain.

I uploaded a metatdata.json to use to drive: https://drive.google.com/file/d/1137jNjQZYdSNOkXtk42LYv8yD-kOkcHP/view?usp=sharing

I used: curl -H "Content-Type: application/json" -d '{"id":"1", "jsonrpc":"2.0", "method": "state_getMetadata", "params":[]}' http://localhost:9933 > metadata.json to get it.

Copy link
Contributor

Choose a reason for hiding this comment

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

What was the specific problem with using the endpoint?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, if it is unavoidable to use this approach, then we should add a script that fetches the metadata and saves it into the json file, and add that to the overall process, deleting it at the end (or just .gitignore it)

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 kept on thinking it was a file

Error: Cannot find module '/Users/eric/polymesh-sdk/ws:/localhost:9944'

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 had to check again since the error stripped a / it seems. the meta one seems fine though

Thats with:

"generate:defs": "ts-node --skip-project node_modules/.bin/polkadot-types-from-defs --endpoint metadata.json --package polymesh-types --input ./src/polkadot/"

Copy link
Contributor

Choose a reason for hiding this comment

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

Created a task to track this

Copy link
Contributor

Choose a reason for hiding this comment

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

I ended up removing the task because there are other scripts that would benefit from having the metadata stored in a file. I created a task to automate fetching and storing the metadata as part of the typegen process

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 have fetch-metadate as part of the typegen now, and it will get .gitignored

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 relies on curl being present, but I think its reasonable to expect being in the path in this day and age on a dev machine.

Might be nice to use a node dependency though.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll create a task for that

package.json Outdated Show resolved Hide resolved
@@ -27,7 +28,7 @@ websocket.onmessage = message => {
const registry = new TypeRegistry();

const metadata = new Metadata(registry, JSON.parse(message.data).result);
const modules = metadata.asLatest.modules;
const modules = metadata.asLatest.pallets;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There are other changes we should investigate: https://polymath.atlassian.net/browse/DA-305

src/api/entities/Venue/index.ts Show resolved Hide resolved

/**
* @hidden
* Needed to handle snake case from rpc request
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This took me a long time to figure out what was happening. I was in a circle of fixing things while breaking other things for longer than I'd like to admit.

This works, but really we should figure out why RPC stuff didn't get updated. I put the word basic in for copy pasta for snake case stuff.

I am hoping if we figure out the generate script stuff it will eliminate the need for these (more in conversion).

Copy link
Contributor

Choose a reason for hiding this comment

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

If we can't fully get rid of the schema then I think we should create some sort of manual mapping from schema types to metadata types, and then run a script that transforms the rpc portion of the schema with said mapping

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The schema does play a role, but the actual types used are coming from augment-api-rpc and augment-api-query.

I think its weird the RPC one looks at polymesh-types/polymesh for its imports, the query one gets them from @polkadot/types/lookup.

There might be a reason for the divergence, but I'd think we can transform the rpc file like you are suggesting.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In that file I changed Authorization to PolymeshPrimitivesAuthorization by hand and that seemed to work.

I think changing that file is the right approach. I am going to give that a shot

src/api/procedures/setTransferRestrictions.ts Outdated Show resolved Hide resolved
src/api/procedures/setTransferRestrictions.ts Outdated Show resolved Hide resolved
src/api/procedures/setTransferRestrictions.ts Show resolved Hide resolved
src/api/procedures/setTransferRestrictions.ts Show resolved Hide resolved
/**
* @hidden
*
* Creates an instance of a type as registered in the polymeshApi instance
*/
public createType<K extends keyof InterfaceTypes>(type: K, params: unknown): InterfaceTypes[K] {
public createType<K extends keyof InterfaceTypes>(type: K, params: unknown): any {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

They made a change that was supposed to make the createType API smarter, but now InterfaceTypes[K] exceeds TypeScript stack size, and might be infinite recursion even.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can copy the signature of the original createType to avoid this use of any

Copy link
Contributor

@monitz87 monitz87 left a comment

Choose a reason for hiding this comment

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

GJ overall, this was a massive endeavor

"generate:defs": "ts-node --skip-project node_modules/.bin/polkadot-types-from-defs --package polymesh-types --input ./src/polkadot",
"generate:meta": "ts-node --skip-project node_modules/.bin/polkadot-types-from-chain --package polymesh-types --endpoint ws://localhost:9944 --output ./src/polkadot --strict",
"generate:defs": "ts-node --skip-project node_modules/.bin/polkadot-types-from-defs --endpoint metadata.json --package polymesh-types --input ./src/polkadot/",
"generate:meta": "ts-node --skip-project node_modules/.bin/polkadot-types-from-chain --endpoint metadata.json --package polymesh-types --output ./src/polkadot --strict",
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, if it is unavoidable to use this approach, then we should add a script that fetches the metadata and saves it into the json file, and add that to the overall process, deleting it at the end (or just .gitignore it)

Comment on lines 3 to 7
import {
PolymeshPrimitivesComplianceManagerAssetCompliance,
PolymeshPrimitivesConditionTrustedIssuer,
} from '@polkadot/types/lookup';
import { AssetComplianceResult } from 'polymesh-types/types';
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are some types being changed but others aren't?

Copy link
Contributor

Choose a reason for hiding this comment

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

We should try sticking with only the new types. Try removing the types part of the schema (since we still need the rpc part) and see how it goes. It should fix issues with mismatched types like VenueDetails which is Bytes in the metadata but Text in the schema.

Let me know how it goes. If it works we can modify the fetch-definitions script to remove the types

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Anything coming from RPC wasn't updated. Will try out your suggestion and see how it goes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

After some experimenting it seems like we should be able to take out the types, but we'll need to figure out CountryCode generate section since that needs types.

We'll also need to go through all the createType calls and make sure none are using the old types either.

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 added in a script that deals with Authorization and that seems to work.

The Result ones seem to live in src/polkadot/polymesh/types.ts. I will take a look in the morning to see if they can get similar treatment.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Seems like the script isn't good after all. The types and tests are happy, but when I tried it out it seems like the snake case type is returned (undefined errors). Using snake case for the field resolves it, so for now I am going to switch back to having another method for handling RPC authorization stuff.

Copy link
Contributor

Choose a reason for hiding this comment

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

The country code stuff should be able to be generated from the metadata. I'll create a task for that

scripts/generateTxTags.js Outdated Show resolved Hide resolved
scripts/transactions.json Show resolved Hide resolved
src/utils/conversion.ts Outdated Show resolved Hide resolved
src/utils/conversion.ts Outdated Show resolved Hide resolved
src/utils/conversion.ts Outdated Show resolved Hide resolved
src/utils/conversion.ts Outdated Show resolved Hide resolved
src/utils/conversion.ts Outdated Show resolved Hide resolved
Adds a post process script that will transform the augment-api-rpc so
that Authorization and IdentityId will be brough in from
`@polkadot/types/lookup`
monitz87 and others added 8 commits May 23, 2022 22:40
Also fix compilation errors that arise in sandbox
…data-v14

Adapt TxTag generating script to metadata v14
Also fixed documentHash conversion util tests
BREAKING CHANGE: 🧨 throw an error instead of a warning when the chain versions aren't
within the expected ranges
@@ -0,0 +1 @@
curl -H "Content-Type: application/json" -d '{"id":"1", "jsonrpc":"2.0", "method": "state_getMetadata", "params":[]}' http://localhost:9933 > metadata.json
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
curl -H "Content-Type: application/json" -d '{"id":"1", "jsonrpc":"2.0", "method": "state_getMetadata", "params":[]}' http://localhost:9933 > metadata.json
curl -H "Content-Type: application/json" -d '{"id":"1", "jsonrpc":"2.0", "method": "state_getMetadata", "params":[]}' http://localhost:9933 > metadata.json

@@ -1,32 +1,32 @@
import {
BTreeSetStatType,
BTreeSetStatUpdate,
BTreeSetTransferCondition,
PolymeshPrimitivesTransferComplianceTransferCondition,
} from '@polkadot/types/lookup';
import BigNumber from 'bignumber.js';
import { TransferCondition, TxTag, TxTags } from 'polymesh-types/types';
Copy link
Contributor

Choose a reason for hiding this comment

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

TxTag and TxTags should be imported from ~/types

Comment on lines 108 to 112
if (type === TransferRestrictionType.Count) {
condition = { type, value: (r as CountTransferRestrictionInput).count };
} else {
condition = { type, value: (r as PercentageTransferRestrictionInput).percentage };
}
Copy link
Contributor

Choose a reason for hiding this comment

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

What you can do is

let value: BigNumber;

if ('count' in inputCondition) {
  value = inputCondition.count;
} else {
  value = inputCondition.percentage;
}

You could stick this in a ternary as well but I'd rather not

@polymath-eric polymath-eric marked this pull request as ready for review May 26, 2022 21:33
polymath-eric and others added 7 commits May 26, 2022 18:22
* refactor: 💡 Eliminate RPC specific conversion functions

Modify the schema object to make its types compatible with the rest of
the api. This elminates snake case conversion functions to DRY the code
base

* fix: 🐛 Fix asset.settlement.canTransfer method

Update types to use PolymeshPrimitivesIdentityId instead of IdentityId
…dation

feat: 🎸 validate chain spec version when connecting
@sonarcloud
Copy link

sonarcloud bot commented May 30, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@polymath-eric polymath-eric merged commit 2450467 into alpha May 30, 2022
@polymath-eric polymath-eric deleted the DA-179/upgrade-5.0.0 branch May 30, 2022 20:32
@monitz87
Copy link
Contributor

🎉 This PR is included in version 14.0.0-alpha.15 🎉

The release is available on:

Your semantic-release bot 📦🚀

@monitz87
Copy link
Contributor

🎉 This PR is included in version 14.0.0-alpha.16 🎉

The release is available on:

Your semantic-release bot 📦🚀

@monitz87
Copy link
Contributor

🎉 This PR is included in version 15.0.0-alpha.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@monitz87
Copy link
Contributor

🎉 This PR is included in version 15.0.0-beta.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@monitz87
Copy link
Contributor

🎉 This PR is included in version 15.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants