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

Conversation

@fubuloubu
Copy link
Contributor

Fixes: #4216

@parity-cla-bot
Copy link

It looks like @fubuloubu signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

/// NOTE: No checking goes on to ensure this is a real public key. Only use it if
/// you are certain that the array actually is a pubkey. GIGO!
pub fn from_full(data: [u8; 65]) -> Self {
let raw_key = &data[1..];
Copy link
Member

Choose a reason for hiding this comment

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

Whitespaces

@gavofyork gavofyork added this to the polkadot-0.7.0 milestone Nov 27, 2019
@fubuloubu
Copy link
Contributor Author

Also, as noted in #4216 (comment) using keccak256 instead of blake2_256 in AccountId hashing logic would make this more Ethereum-compatible, but as I noted in my response it would be better if AccountId wasn't assumed 32 bytes (instead just Sized) and was user configurable with how addresses are encoded.

I could add keccak_256 to this PR though.

@gavofyork
Copy link
Member

gavofyork commented Nov 27, 2019

AccountId is:

type AccountId: Parameter + Member + MaybeSerializeDeserialize + Debug + MaybeDisplay + Ord + Default;

So basically, you can have it be any type you want. There's no need for this PR, though since it doesn't really break anything and makes things strictly more compatible I'm ok merging it. But you can in any case make a 20-byte AccountId and use sha256, keccak256, ripemd-160 or whatever else if you really want and you can make a new Signature type which is compatible with it (and a new MultiSignature if you want to support other crypto at the same time, too). That's the beauty of FRAME - it provides the abstractions to let you decide without needing the code to be in the core.

@gavofyork
Copy link
Member

For now, though, some of the CI is failing and will need to be fixed before it can be merged.

@fubuloubu
Copy link
Contributor Author

[Y]ou can in any case make a 20-byte AccountId and use sha256, keccak256, ripemd-160 or whatever else if you really want and you can make a new Signature type which is compatible with it (and a new MultiSignature if you want to support other crypto at the same time, too).

Ah, well I will have to try a different method of doing what I want. I was using MultiSigner and MultiSignature, which builds in that presumption of the blake2 hash, and I assumed there was no other way to do it, but I see where I am wrong though. This PR is definitely still foundational however.

Will fix CI issues.

@fubuloubu
Copy link
Contributor Author

As noted above, the 120 char line limit is failing because I made a hex string larger. It doesn't seem like I should be splitting it up, so it is probably best to ignore this error?

The other two errors do not seem to be my fault, as far as I can tell.

Please advise.

@gavofyork gavofyork merged commit 657736e into paritytech:master Nov 27, 2019
@fubuloubu fubuloubu deleted the change-secp256k1-accountId branch November 28, 2019 05:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Hash raw secp256k1 Public Keys instead of compressed ones

4 participants