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: unsigned transaction metadata #158

Merged
merged 14 commits into from
Dec 8, 2021
Merged

Conversation

TarikGul
Copy link
Member

@TarikGul TarikGul commented Dec 8, 2021

A bug was introduced in 1.3.0 where I changed the metadataRpc field in the constructed unsigned transactions to point to registry.metadata.toHex(). For further details see polkadot-js/api#4293.

This PR introduces a fix in defineMethod where I generate the metadata outside of registry.setMetadata() and save it to its own variable so that generated metadata can then be used to be assigned to metadataRpc. This is important because it allows for a minimized size in transaction when using asCallsOnlyArg without tampering with the integrity metadata itself.

I also added a test for getRegistryBase that ensures when using asCallsOnlyArg that it will also minimize the metadata within the registry.

@lcovar
Copy link

lcovar commented Dec 8, 2021

Can confirm that this change will fix the issue in polkadot-js/api#4293. Thanks.

How soon can we expect a release after this is merged?

@TarikGul
Copy link
Member Author

TarikGul commented Dec 8, 2021

@lcovar Nice! Thanks for checking it out. We will release a patch tomorrow with these fixes.

@doodles92
Copy link

Hi @TarikGul we're from the Bitgo team. Can we confirm that this will go out tomorrow as it's blocking our development to onboard DOT to our platform.
CC: @lcovar

@TarikGul
Copy link
Member Author

TarikGul commented Dec 8, 2021

@doodles92 Yes can confirm this will be patched immediately, can't say when tomorrow exactly, but the goal is tomorrow regardless.

* This similar method to txwrapper-registry is used here to
* test getRegistryBase.
*/
export const knownChainProperties = substrateSS58Registry.reduce(
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we re-export this via txwrapper-registry so its not duplicated?

Copy link
Member Author

@TarikGul TarikGul Dec 8, 2021

Choose a reason for hiding this comment

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

It complains about circular dependencies. Being that the registry uses core, if core uses registry it wont build correctly.

@TarikGul TarikGul merged commit acd176b into main Dec 8, 2021
@TarikGul TarikGul deleted the tarik-fix-metadata-unsigned branch December 8, 2021 10:43
@TarikGul
Copy link
Member Author

TarikGul commented Dec 8, 2021

@lcovar , @doodles92 - v1.3.2 has been released, and reflects these changes.

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