Add Pico support to the vector pallet. #761
Conversation
| let updater = Updater::<T>::get(); | ||
| let pico_updater = PicoUpdater::<T>::get(); | ||
| let sp1_updater = Updater::<T>::get(); | ||
| let (updater, _updater_type) = if H256(sender) == pico_updater { |
There was a problem hiding this comment.
Is this _updater_type used anywhere?
|
|
||
| let vk_bin = include_bytes!("../pico_vk.bin"); | ||
| let vk = gnark_bn_verifier::vk::Groth16VKey::try_from(vk_bin.as_slice()); | ||
| ensure!(vk.is_ok(), Error::<T>::BadPicoVkKey); |
There was a problem hiding this comment.
Here we could use let..else pattern together with return Error to avoid the unwrap down below.
|
|
||
| let public_inputs = vkh | ||
| .chunks(32) | ||
| .map(|e| Fr::from_slice(e).unwrap()) |
There was a problem hiding this comment.
scary unwrap here
| let updater = Updater::<T>::get(); | ||
| let pico_updater = PicoUpdater::<T>::get(); | ||
| let sp1_updater = Updater::<T>::get(); | ||
| let (updater, updater_type) = if H256(sender) == pico_updater { |
There was a problem hiding this comment.
Not an issue but style-wise would a match operator be more elegant in this instance?
| /// Updater that can submit updates | ||
| #[pallet::storage] | ||
| #[pallet::getter(fn pico_updater)] | ||
| pub type PicoUpdater<T: Config> = StorageValue<_, H256, ValueQuery>; |
There was a problem hiding this comment.
Not an issue but we should keep on mind that we need to set these storage (PicoUpdater, PicoVerificationKey) once we do the runtime upgrade
| /// Emit new updater. | ||
| NewUpdater { old: H256, new: H256 }, | ||
| /// Emit new updater. | ||
| NewPicoUpdater { old: H256, new: H256 }, |
There was a problem hiding this comment.
Could you move the newly created events to the bottom? thank you
| sp1-verifier = { version = "5.0.0", default-features = false } | ||
| alloy-sol-types = { version = "0.8.12", default-features = false } | ||
| hex = { version = "*", default-features = false } | ||
| gnark-bn-verifier = { git = "https://github.com/availproject/gnark-bn-verifier", branch = "no-std", default-features = false } |
There was a problem hiding this comment.
Let's move all the new dependencies to the main cargo.toml file and here just reference it with { workspace = true, default-features = false }. If you have the will could you do the same for sp1-verifier and alloy-sol-types?
| Weight::zero() | ||
| } | ||
| } | ||
| enum UType { |
There was a problem hiding this comment.
Not an issue but style-wise it might make sense to move this type definition to the top of the pallet mod section.
| .collect::<Vec<_>>(); | ||
|
|
||
| let is_valid = groth16_proof.verify(&vk, public_inputs.as_slice()); | ||
|
|
There was a problem hiding this comment.
scary empty space
Pull Request type
Please add the labels corresponding to the type of changes your PR introduces:
Description
Adding support for Pico verification to Vector pallet. Includes a forked external crate, that may need to be upstreamed.
Related Issues
Testing Performed
The main test was added to test the feature. Some more would probably also need to be added, to cover everything, but this is a good start.
Checklist
cargo test.cargo fmt.cargo build --releaseandcargo build --release --features runtime-benchmarks.cargo clippy.