feat(shielded-pool-extension): PIR + epoch-nullifier extension (PoC)#74
Conversation
… attestation relaxed)
| address withdrawV = address(new WithdrawVerifier()); | ||
| address insertionV = address(new InsertionVerifier()); | ||
| MockERC20 token = new MockERC20("USD Coin", "USDC", 6); | ||
| // withdrawInsertionVerifier (k=1) has no circuit yet; reuse the k=2 |
There was a problem hiding this comment.
This wires the k=2 InsertionVerifier into the withdrawInsertionVerifier slot. withdraw() supplies only one nullifier to _verifyInsertionAndAdvance, so the verifier receives 4 public inputs, but the generated k=2 insertion verifier expects 5 (NUMBER_OF_PUBLIC_INPUTS - PAIRING_POINTS_SIZE). With this deployment, any real withdraw path will revert inside the verifier before returning false.
Since the README/contract describe withdraw as part of the spend path, this should either deploy a real k=1 insertion verifier or keep withdraw out of the real-verifier deployment until that circuit exists.
rymnc
left a comment
There was a problem hiding this comment.
general pass; didn't inspect logic too deeply, focused on minor things
will do a bigger review a little later today or tomorrow
| /// by feeding it directly into the commitment preimage, so a deposit always | ||
| /// mints a note tagged with the live epoch. | ||
| /// | ||
| /// Implementation shortcut: the parent's KYC attestation-membership check is |
There was a problem hiding this comment.
why is this a shortcut, seems simple enough to add here
| /// Forge auto-deploys + links the contract libraries (ZKTranscriptLib in the | ||
| /// verifiers; PoseidonT3 + LeanIMT in the pool). Writes `deployments.toml` | ||
| /// for the test to read. | ||
| contract Deploy is Script { |
There was a problem hiding this comment.
this is not how the deployments.toml is generated, this deploy script should inherit Config, see https://github.com/ethereum/iptf-pocs/blob/b2d47a1a33b6086b853beec844f8dce693b94d48/pocs/civic-participation/resilient-civic-participation/contracts/script/Deploy.s.sol#L18
| IShieldedPoolExt::new(self.pool, &self.provider) | ||
| } | ||
|
|
||
| fn convert_receipt(receipt: &alloy::rpc::types::TransactionReceipt) -> TxReceipt { |
There was a problem hiding this comment.
maybe replace with
impl From<alloy::rpc::types::TransactionReceipt> for TxReceipt {
fn from(alloy_receipt: alloy::rpc::types::TransactionReceipt) -> Self {
/// whatever code u have here
}
}| //! PoC note: `root()` and `merkle_path()` recompute the occupied levels on each | ||
| //! call (O(occupied) hashing); fine at PoC scale, not optimized for Visa-scale | ||
| //! epochs. | ||
| //! |
There was a problem hiding this comment.
what is the suggested solution for this? would be nice to include it with the comment
| /// PoC: the cleartext adapter sends the leaf index to the server, which learns | ||
| /// it. Slice 2 swaps in a real PIR backend that computes the sibling-node offsets |
There was a problem hiding this comment.
what is slice 2? probably not best to send leaf index to the server
| pub trait RootVerifier { | ||
| /// Verify `proof` (an `eth_getProof` result for one slot) and return the | ||
| /// verified storage value. | ||
| fn verify_storage(&self, proof: &EIP1186AccountProofResponse) -> Result<alloy::primitives::B256, RootVerifierError>; | ||
| } |
There was a problem hiding this comment.
just an engineering note: ports should rely on types declared in the same file, or in the domains module. here we are depending on an external crate in the args and return type; and here you are creating a contract that the adapter has to use this crate too - fine for poc, but something to note
| @@ -0,0 +1,82 @@ | |||
| //! Light-client root-verification integration test (Slice 3). | |||
There was a problem hiding this comment.
can we remove comments with "Slice *", confusing to read
| const DEV_KEY: &str = "0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80"; | ||
|
|
||
| #[tokio::test] | ||
| #[ignore = "spawns anvil; run with --ignored"] |
There was a problem hiding this comment.
we should remove this #ignore attribute
…om impl, zeroize, Config deploy
|
Thanks for the pass! Pushed 2 commits (e403c07, 4a75622) addressing this round. @oskarth (withdraw insertion verifier): Fixed. Pulled the insertion loop into a shared @rymnc (resolved in code): deleted the 5 unused stub files; removed the "Slice N" comments; @rymnc (answers to the questions/notes):
|
Implements the (already-reviewed) SPEC.
Here: extended circuits (deposit, chain_update IVC, transfer/withdraw, insertion),
ShieldedPoolExt.soltwo-proof spend path, the Rust off-chain stack (state replica, bb-prover, alloy adapter, SimplePIR commitment-path client), and a self-contained on-chain e2e (deposit → rollover → two-proof transfer with real proofs, ~50 s).Test:
Shortcuts (see README): SimplePIR stands in for InsPIRe (no portable Rust impl; not silent-preprocessing); KYC attestation relaxed in the deposit circuit; light-client root verification (Slice 3) and withdraw's k=1 insertion verifier are follow-ups. Research prototype, not production.