Skip to content

Custom AccountId32 impl, remove substrate deps#1010

Merged
ascjones merged 4 commits intomasterfrom
aj/remove-transcode-substrate-deps
Mar 10, 2023
Merged

Custom AccountId32 impl, remove substrate deps#1010
ascjones merged 4 commits intomasterfrom
aj/remove-transcode-substrate-deps

Conversation

@ascjones
Copy link
Copy Markdown
Collaborator

@ascjones ascjones commented Mar 9, 2023

Removes the substrate dependencies from contract-transcode.

We copy the AccountId32 type from subxt: https://github.com/paritytech/subxt/blob/ce0a82e3227efb0eae131f025da5f839d9623e15/subxt/src/utils/account_id.rs, with a few modifications so that it can work in place of sp_core::crypto::AccountId32

Comment on lines +31 to +35
/// This has been copied from `subxt::utils::AccountId32`, with some modifications:
///
/// - Custom [`scale_info::TypeInfo`] implementation to match original substrate type.
/// - Made `to_ss58check` public.
/// - Implemented `TryFrom<&'a [u8]>`.
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I considered just importing subxt as a dependency and using this type directly, however the above modifications were required.

The other way was to implement a wrapper type, but I decided to copy the code across wholesale and make the modifications. This has the advantage of not pulling in the whole of subxt just for this type.

@ascjones
Copy link
Copy Markdown
Collaborator Author

ascjones commented Mar 9, 2023

Clippy CI failure fixed in #1011

@deuszx
Copy link
Copy Markdown
Contributor

deuszx commented Mar 9, 2023

Thanks @ascjones , this should solve our problems.

Nitpick: Maybe include a short explanation as of why we're doing this.

Copy link
Copy Markdown
Contributor

@SkymanOne SkymanOne left a comment

Choose a reason for hiding this comment

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

I assume the maths for account derivation is correct :)

@ascjones
Copy link
Copy Markdown
Collaborator Author

I assume the maths for account derivation is correct :)

Indeed, all copied from substrate. Though would be nice to have some tests for that...

@ascjones
Copy link
Copy Markdown
Collaborator Author

I assume the maths for account derivation is correct :)

Indeed, all copied from substrate. Though would be nice to have some tests for that...

Actually there is already a test at the bottom of the file that checks those using the account keyring.

@ascjones ascjones merged commit 4ccd237 into master Mar 10, 2023
@ascjones ascjones deleted the aj/remove-transcode-substrate-deps branch March 10, 2023 15:58
This was referenced Mar 10, 2023
@SkymanOne
Copy link
Copy Markdown
Contributor

contract-transcode build was failing due to missing features in primitive-types dependency. See fix here - d89ca99

[dev-dependencies]
assert_matches = "1.5.0"
ink = "4.0.1"
sp-core = { version = "18.0.0", default-features = false }
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is default-features = false needed here?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

No, probably not.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Tacked it on to #1026

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants