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

Use peerDependency or some other way to decouple parachain project type dependency from txwrapper #47

Closed
xlc opened this issue Jan 13, 2021 · 1 comment · Fixed by #48
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@xlc
Copy link
Contributor

xlc commented Jan 13, 2021

I created a simple dependency diagram: https://dreampuf.github.io/GraphvizOnline/#digraph%20G%20%7B%0A%20%20%20%20app%20-%3E%20txwrapper_acala%0A%20%20%20%20txwrapper_acala%20-%3E%20txwrapper_core%0A%20%20%20%20txwrapper_acala%20-%3E%20txwrapper_registry%0A%20%20%20%20txwrapper_registry%20-%3E%20apps_config%0A%20%20%20%20apps_config%20-%3E%20acala_js%0A%20%20%20%20apps_config%20-%3E%20polkadot_js%0A%7D

As you can see, when acala introduce new types, we will update acala.js, and then apps-config needs update then txwrapper-registry needs update, wxwrapper-acala needs update. This will take time definitely doesn't scale.

Acala can maintain txwrapper-acala but we will still depends on txwrapper-registry

Should have a way to allow user of txwrapper to specify the parachain project type / npm package without requiring update all the intermediate dependencies

@emostov emostov added enhancement New feature or request good first issue Good for newcomers labels Jan 13, 2021
@emostov
Copy link
Contributor

emostov commented Jan 13, 2021

Yes, I definitely agree with this idea. To build on it, I think dependance on txwrapper-registry should be avoided when possible because it will require waiting for a cascade of package updates as you clearly point out. Instead we should direct txwrapper maintainers to define their own getRegistry and import their chain specific type package directly.

This should be resolved by #45.

Below is a very rough example of how that may work

// `getRegistry` implementation for a hypothetical txwrapper for a frame based chain.
// This would mean that the txwrapper would not have to require txwrapper-registry and instead can update
// its type package directly as opposed to having to wait for apps-config to update
import { typesBundle, typeSpec, typesChain } from '@frame-based-chain/polkadot-js-types'

 // This interface could be exported from txwrapper-core
export interface GetRegistryOpts {
	/**
	 * Runtime specName
	 */
	specName: keyof typeof knownChainProperties;
	/**
	 * chainName
	 */
	chainName: string;
	/**
	 * Runtime specVersion
	 */
	specVersion: number;
	/**
	 * SCALE encoded runtime metadata as a hex string
	 */
	metadataRpc: string;
	/**
	 * Optionally specify the chain properties .
	 */
	properties?: ChainProperties;
}

export function getRegistry({
	specName,
	chainName,
	specVersion,
	metadataRpc,
	properties,
}: GetRegistryOpts): TypeRegistry {
	// Polkadot, kusama, and westend have known types in the default polkadot-js registry. If we are
	// dealing with another network, use the apps-config types to fill the registry.
	const registry = new TypeRegistry();
	registry.setKnownTypes({
		typesBundle,
		typesChain,
		typesSpec,
	});

	return getRegistryBase({
		chainProperties: properties || knownChainProperties[specName],
		// `getSpecTypes` is used to extract the chain specific types from the registries `knownTypes`
		specTypes: getSpecTypes(registry, chainName, specName, specVersion),
		metadataRpc,
	});
}

@emostov emostov self-assigned this Jan 20, 2021
emostov added a commit that referenced this issue Jan 20, 2021
Closes #47, Closes #46, Closes #45
- Transition packages to use their own getRegistry function in order to decrease deps and supply chain waits for types
- Add a chain builder example for how to create local getRegistry
- remove apps-config dep for txwrapper-orml to reduce package size; replaced with acala types as a dev dep
emostov added a commit that referenced this issue Jan 21, 2021
* feat: Use local `getRegistry` functions

Closes #47, Closes #46, Closes #45
- Transition packages to use their own getRegistry function in order to decrease deps and supply chain waits for types
- Add a chain builder example for how to create local getRegistry
- remove apps-config dep for txwrapper-orml to reduce package size; replaced with acala types as a dev dep

* Apply suggestions from code review

Co-authored-by: Tarik Gul <47201679+TarikGul@users.noreply.github.com>

* Use 1) for md ordered ls

* Update examle code

* Clean up

* Use ^ versioning

Co-authored-by: Tarik Gul <47201679+TarikGul@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants