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

Remove libsecp256k1 dependency of ink_eth_compatibility #1068

Closed
cmichi opened this issue Dec 7, 2021 · 9 comments · Fixed by #1233
Closed

Remove libsecp256k1 dependency of ink_eth_compatibility #1068

cmichi opened this issue Dec 7, 2021 · 9 comments · Fixed by #1233
Assignees
Labels
A-ink_env [ink_env] work item

Comments

@cmichi
Copy link
Collaborator

cmichi commented Dec 7, 2021

We need the libsecp256k1 dependency only in the to_eth_address function, for getting the uncompressed 65-byte public key from the ECDSA public key.

Ways to remove the dep would be to either add a new API function in the contracts pallet, find another no-std compatible crate, or extract the functionality from either libsecp256k1 or secp256k1 into ink_eth_compatibility.

@cmichi cmichi added the A-ink_env [ink_env] work item label Dec 7, 2021
@cmichi cmichi changed the title Remove libsecp256k1 dependency of ink_env_compatibility Remove libsecp256k1 dependency of ink_eth_compatibility Dec 7, 2021
@cmichi
Copy link
Collaborator Author

cmichi commented Mar 2, 2022

@athei @agryaznov

There is not yet an issue for this in pallet-contracts, but I think we should implement it there. This would enable us to get rid of the libsecp256k1 dependency in ink!, thus making contracts using the ink_eth_compatibility smaller.

I can imagine having e.g. sth like a seal_to_eth_address(ecdsa_compressed_public_key: [u8; 32]).

That's the code which would need to be moved to this new pallet-contracts function: https://github.com/paritytech/ink/blob/master/crates/eth_compatibility/src/lib.rs#L99-L119.

@athei
Copy link
Contributor

athei commented Mar 7, 2022

I agree. We should move this to the pallet. We shouldn't need crypto inside the contract for the most basic stuff.

@athei
Copy link
Contributor

athei commented Mar 15, 2022

I think there should be some host function available for the runtime to offload that to. Search for that here: https://github.com/paritytech/substrate/blob/master/primitives/io/src/lib.rs

If is not available there we should depend on k256 in pallet contracts.

@agryaznov
Copy link
Contributor

I think there should be some host function available for the runtime to offload that to. Search for that here: https://github.com/paritytech/substrate/blob/master/primitives/io/src/lib.rs

Haven't found appropriate function there, will move current impl dependent on libsecp256k into pallet.

@athei
Copy link
Contributor

athei commented Mar 16, 2022

I think there should be some host function available for the runtime to offload that to. Search for that here: https://github.com/paritytech/substrate/blob/master/primitives/io/src/lib.rs

Haven't found appropriate function there, will move current impl dependent on libsecp256k into pallet.

We are trying to move away from that crate. Please implement with the k256 crate. Here is a PR where this is done for the beefy-mmr crate: paritytech/substrate#10883

It also needs a conversion to an ETH address. So maybe you can move the pub -> address function to some more general crate and use it from both places.

@agryaznov
Copy link
Contributor

We now have seal_ function for this in pallet_contract. Do we need to implement also some fn to be used from inside ink! contracts? @cmichi

@athei
Copy link
Contributor

athei commented Apr 24, 2022

Of course. Your linked PR should not just remove the dependency but replace it with a call of the seal function.

@agryaznov
Copy link
Contributor

Do we really need this ink_eth_compatibility crate after all? It isn't being used anywhere, and only has one more fn aside from to_eth_address(), which is to_default_account_id() and looks like something that should be put out of ink as well (probably to the same ecdsa trait extension in frame_support).

@athei
Copy link
Contributor

athei commented Apr 25, 2022

Yeah I don't think we need this extra crate anymore. I think we only had it to avoid pulling in the crypto dependency. But better wait for @cmichi to comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ink_env [ink_env] work item
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants