Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Remove in-tree max-encoded-len and use the new SCALE codec crate instead #9163

Merged
13 commits merged into from
Jul 5, 2021

Conversation

Xanewok
Copy link
Contributor

@Xanewok Xanewok commented Jun 21, 2021

Helps with #9140.

This PR uses the new version (git patch) of parity-scale-codec that's merged with the in-tree max-encoded-len crate. Since we don't plan to use/release this crate, this aims to gauge if using a hypothetical new release of the scale codec crate keeps the build running and tests passing, so that we can safely migrate and remove the max-encoded-len crate from the tree altogether.

It's worth noting that the in-tree version had some explicit impls for primitive hash types that but it's cleaner to implement these directly in the primitive-types crate (see fe797ff comment).

If the CI is green and this is accepted by reviews, we will need to release the locally patched crates to crates.io and then switch to them before this PR is merged.

Associated changes:
parity-scale-codec: https://github.com/paritytech/parity-scale-codec/compare/igor-max-encoded-len
primitive-types: https://github.com/paritytech/parity-common/compare/igor-prim-types-new-codec

polkadot companion: paritytech/polkadot#3412

@Xanewok Xanewok added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Jun 21, 2021
@Xanewok Xanewok force-pushed the igor-use-codec-with-bounded branch from 6c8ca1f to 797dc90 Compare June 21, 2021 13:19
@Xanewok Xanewok added the D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit label Jun 21, 2021
@coriolinus
Copy link
Contributor

With paritytech/parity-scale-codec#268 merged, MaxEncodedLen now exists on master of that repo.

Cargo.toml Outdated Show resolved Hide resolved
Copy link
Contributor

@coriolinus coriolinus left a comment

Choose a reason for hiding this comment

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

Seems fine once the tests all pass.

@Xanewok
Copy link
Contributor Author

Xanewok commented Jun 23, 2021

As it turns out, if we want to proceed with using the pre-release, we need to explicitly select it (it's not considered SemVer-compatible), so this also requires:

@Xanewok
Copy link
Contributor Author

Xanewok commented Jun 28, 2021

I'll fix the merge conflict and update the UI test once #9163 (comment) progresses.

@Xanewok Xanewok force-pushed the igor-use-codec-with-bounded branch from f2d38cc to 6719d01 Compare July 4, 2021 21:17
Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

On a side note, please stop using force pushes ;)

Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

On a side note, please stop using force pushes ;)

@Xanewok Xanewok changed the title WIP: Remove in-tree max-encoded-len and use the new SCALE codec crate instead Remove in-tree max-encoded-len and use the new SCALE codec crate instead Jul 5, 2021
@Xanewok
Copy link
Contributor Author

Xanewok commented Jul 5, 2021

https://gitlab.parity.io/parity/substrate/-/jobs/997661#L332

error: failed to select a version for `impl-codec`.
    ... required by package `primitive-types v0.9.0`
    ... which is depended on by `parity-util-mem v0.9.0`
    ... which is depended on by `kvdb v0.9.0`
    ... which is depended on by `kvdb-memorydb v0.9.0`
    ... which is depended on by `kvdb-web v0.9.0`
    ... which is depended on by `substrate-browser-utils v0.9.0 (/builds/parity/substrate/utils/browser)`
    ... which is depended on by `polkadot-cli v0.9.8 (/builds/parity/substrate/polkadot/cli)`
    ... which is depended on by `polkadot v0.9.8 (/builds/parity/substrate/polkadot)`
versions that meet the requirements `=0.5.0` are: 0.5.0
all possible versions conflict with previously selected packages.
  previously selected package `impl-codec v0.5.1`
    ... which is depended on by `sp-core v3.0.0 (/builds/parity/substrate/primitives/core)`
    ... which is depended on by `beefy-gadget v0.1.0 (https://github.com/paritytech/grandpa-bridge-gadget?branch=master#10872bb2)`
    ... which is depended on by `beefy-gadget-rpc v0.1.0 (https://github.com/paritytech/grandpa-bridge-gadget?branch=master#10872bb2)`
    ... which is depended on by `polkadot-rpc v0.9.8 (/builds/parity/substrate/polkadot/rpc)`
    ... which is depended on by `polkadot-service v0.9.8 (/builds/parity/substrate/polkadot/node/service)`
    ... which is depended on by `polkadot-cli v0.9.8 (/builds/parity/substrate/polkadot/cli)`
    ... which is depended on by `polkadot v0.9.8 (/builds/parity/substrate/polkadot)`
failed to select a version for `impl-codec` which could resolve this conflict

That doesn't make any sense, primitive types v0.9.0 has a ^0.5.0 dependency on impl-codec so it should be compatible with our new ^0.5.1 explicitly selected constrain in dec27e2...

Any ideas?

@Xanewok
Copy link
Contributor Author

Xanewok commented Jul 5, 2021

bot merge

@ghost
Copy link

ghost commented Jul 5, 2021

Trying merge.

@ghost ghost merged commit d22f0df into master Jul 5, 2021
@ghost ghost deleted the igor-use-codec-with-bounded branch July 5, 2021 20:37
dvdplm added a commit that referenced this pull request Jul 6, 2021
* master:
  fix staking version in genesis (#9280)
  fix storage info for decl_storage (#9274)
  Authority_discovery: expose assimilate_storage with GenesisBuild (#9279)
  Update CODEOWNERS (#9278)
  Remove in-tree `max-encoded-len` and use the new SCALE codec crate instead (#9163)
  bump a bunch of deps in parity-common (#9263)
  Bump linregress due to security vulnerability (#9262)
  pallet macro: always generate storage info on pallet struct (#9246)
  Less duplication in test code (#9270)
  Add `Chilled` event to staking chill extrinsics (#9250)
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants