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

Add conditions around solidity failure cases for hardhat network only #46

Merged
merged 8 commits into from
Aug 28, 2024
4 changes: 2 additions & 2 deletions go-sdk/internal/sparse-merkle-tree/smt/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@ package smt
import "errors"

var (
// ErrMaxLevelsExceeded is used when a level is larger than the max
ErrMaxLevelsExceeded = errors.New("tree height is larger than max allowed (256)")
// ErrMaxLevelsNotInRange is used when a level is larger than the max
ErrMaxLevelsNotInRange = errors.New("tree height must be larger than zero and less than max allowed (256)")
// ErrNodeIndexAlreadyExists is used when a node index already exists.
ErrNodeIndexAlreadyExists = errors.New("key already exists")
// ErrKeyNotFound is used when a key is not found in the MerkleTree.
Expand Down
4 changes: 2 additions & 2 deletions go-sdk/internal/sparse-merkle-tree/smt/merkletree.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,8 @@ type sparseMerkleTree struct {
}

func NewMerkleTree(db core.Storage, maxLevels int) (core.SparseMerkleTree, error) {
if maxLevels > MAX_TREE_HEIGHT {
return nil, ErrMaxLevelsExceeded
if maxLevels <= 0 || maxLevels > MAX_TREE_HEIGHT {
return nil, ErrMaxLevelsNotInRange
}
mt := sparseMerkleTree{db: db, maxLevels: maxLevels}

Expand Down
67 changes: 67 additions & 0 deletions go-sdk/internal/sparse-merkle-tree/smt/merkletree_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
// Copyright © 2024 Kaleido, Inc.
//
// SPDX-License-Identifier: Apache-2.0
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package smt

import (
"fmt"
"testing"

"github.com/hyperledger-labs/zeto/go-sdk/internal/sparse-merkle-tree/storage"
"github.com/hyperledger-labs/zeto/go-sdk/pkg/sparse-merkle-tree/core"
"github.com/stretchr/testify/assert"
)

type mockStorage struct {
GetRootNodeIndex_customError bool
}

func (ms *mockStorage) GetRootNodeIndex() (core.NodeIndex, error) {
if ms.GetRootNodeIndex_customError {
return nil, fmt.Errorf("nasty error in get root")
}
return nil, storage.ErrNotFound
}
func (ms *mockStorage) UpsertRootNodeIndex(core.NodeIndex) error {
return fmt.Errorf("nasty error in upsert root")
}
func (ms *mockStorage) GetNode(core.NodeIndex) (core.Node, error) {
return nil, nil
}
func (ms *mockStorage) InsertNode(core.Node) error {
return nil
}
func (ms *mockStorage) Close() {}

func TestNewMerkleTreeFailures(t *testing.T) {
db := &mockStorage{}
mt, err := NewMerkleTree(db, 0)
assert.EqualError(t, err, ErrMaxLevelsNotInRange.Error())
assert.Nil(t, mt)

mt, err = NewMerkleTree(nil, 257)
assert.Error(t, err, ErrMaxLevelsNotInRange.Error())
assert.Nil(t, mt)

mt, err = NewMerkleTree(db, 64)
assert.EqualError(t, err, "nasty error in upsert root")
assert.Nil(t, mt)

db.GetRootNodeIndex_customError = true
mt, err = NewMerkleTree(db, 64)
assert.EqualError(t, err, "nasty error in get root")
assert.Nil(t, mt)
}
15 changes: 3 additions & 12 deletions solidity/scripts/deploy_cloneable.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,8 @@ export async function deployFungible(tokenName: string) {
const { deployer, args, libraries } = await verifiersDeployer.deployDependencies();

let zetoFactory;
const opts = {
kind: 'uups',
initializer: 'initialize',
unsafeAllow: ['delegatecall']
};
Comment on lines -11 to -15
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought we needed this for upgradable contracts?

Copy link
Contributor Author

@jimthematrix jimthematrix Aug 27, 2024

Choose a reason for hiding this comment

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

It's accurate that this options object is needed for upgradeable contracts, but here we are deploying the implementation contract that can then be cloned, so no need for these parameters.

What we could do is, demonstrate how the cloned instance can then be turned into an upgradeable contract. But that's making the sample too complex (first deploy the implementation, then deploy the clone factory, then create the clone, and then make it into an upgradeable).

Copy link
Contributor

Choose a reason for hiding this comment

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

sounds good

if (libraries) {
zetoFactory = await getLinkedContractFactory(tokenName, libraries);
opts.unsafeAllow.push('external-library-linking');
} else {
zetoFactory = await ethers.getContractFactory(tokenName)
}
Expand All @@ -24,6 +18,9 @@ export async function deployFungible(tokenName: string) {
await zetoImpl.waitForDeployment();
await zetoImpl.connect(deployer).initialize(...args);

const tx3 = await zetoImpl.connect(deployer).setERC20(erc20.target);
await tx3.wait();

console.log(`ERC20 deployed: ${erc20.target}`);
console.log(`ZetoToken deployed: ${zetoImpl.target}`);

Expand All @@ -36,14 +33,8 @@ export async function deployNonFungible(tokenName: string) {
const { args, libraries } = await verifiersDeployer.deployDependencies();

let zetoFactory;
const opts = {
kind: 'uups',
initializer: 'initialize',
unsafeAllow: ['delegatecall']
};
if (libraries) {
zetoFactory = await getLinkedContractFactory(tokenName, libraries);
opts.unsafeAllow.push('external-library-linking');
} else {
zetoFactory = await ethers.getContractFactory(tokenName)
}
Expand Down
17 changes: 17 additions & 0 deletions solidity/test/lib/deploy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,15 @@ import { ethers } from 'hardhat';
export async function deployZeto(tokenName: string) {
let zeto, erc20, deployer;

// for testing with public chains, skip deployment if
// the contract address is provided
if (process.env.ZETO_ADDRESS && process.env.ERC20_ADDRESS) {
zeto = await ethers.getContractAt(tokenName, process.env.ZETO_ADDRESS);
erc20 = await ethers.getContractAt('SampleERC20', process.env.ERC20_ADDRESS);
deployer = (await ethers.getSigners())[0];
return { deployer, zeto, erc20 };
}

let isFungible = false;
const fungibility = (fungibilities as any)[tokenName];
if (fungibility === 'fungible') {
Expand All @@ -28,6 +37,8 @@ export async function deployZeto(tokenName: string) {
const result = await deployFunc(tokenName);
({ deployer, zetoImpl, erc20, args } = result as any);

// we want to test the effectiveness of the factory contract
// to create clones of the Zeto implementation contract
const Factory = await ethers.getContractFactory("ZetoTokenFactory");
const factory = await Factory.deploy();
await factory.waitForDeployment();
Expand All @@ -49,6 +60,12 @@ export async function deployZeto(tokenName: string) {
}
}
zeto = await ethers.getContractAt(tokenName, zetoAddress);

// set the ERC20 token for the fungible Zeto token
if (isFungible) {
const tx3 = await zeto.connect(deployer).setERC20(erc20.target);
await tx3.wait();
}
}

return { deployer, zeto, erc20 };
Expand Down
86 changes: 46 additions & 40 deletions solidity/test/zeto_anon.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,7 @@
// See the License for the specific language governing permissions and
// limitations under the License.

import hre from 'hardhat';
const { ethers } = hre;
import { ethers, network } from 'hardhat';
import { Signer, BigNumberish, AddressLike, ZeroAddress } from 'ethers';
import { expect } from 'chai';
import { loadCircuit, encodeProof, Poseidon } from "zeto-js";
Expand Down Expand Up @@ -45,6 +44,7 @@ describe("Zeto based fungible token with anonymity without encryption or nullifi
let circuit: any, provingKey: any;

before(async function () {
this.timeout(600000);
Chengxuan marked this conversation as resolved.
Show resolved Hide resolved
let [d, a, b, c] = await ethers.getSigners();
deployer = d;
Alice = await newUser(a);
Expand All @@ -53,9 +53,6 @@ describe("Zeto based fungible token with anonymity without encryption or nullifi

({ deployer, zeto, erc20 } = await deployZeto('Zeto_Anon'));

const tx3 = await zeto.connect(deployer).setERC20(erc20.target);
await tx3.wait();

circuit = await loadCircuit('anon');
({ provingKeyFile: provingKey } = loadProvingKeys('anon'));
});
Expand All @@ -64,7 +61,7 @@ describe("Zeto based fungible token with anonymity without encryption or nullifi
const tx = await erc20.connect(deployer).mint(Alice.ethAddress, 100);
await tx.wait();
const balance = await erc20.balanceOf(Alice.ethAddress);
expect(balance).to.equal(100);
expect(balance).to.be.gte(100);
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel we shouldn't sacrifice the scope of assertions in the tests when running against a hardhat network.

Copy link
Contributor Author

@jimthematrix jimthematrix Aug 27, 2024

Choose a reason for hiding this comment

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

This doesn't impact runs against the hardhat network. Ignition is pretty smart, it only does the caching of the deployed contracts, to save on gas apparently, when deploying against a non-hardhat contract. So the change to check on greater than or equal to is equivalent to equal to when running against the hardhat network.

Here's the abridged output from runs against the hardhat network, you can see the ERC20 contract is re-deployed for every test suite:

  Zeto based fungible token with anonymity without encryption or nullifier
Deploying as upgradeable contracts
ZetoToken deployed: 0x5FC8d32690cc91D4c39d9d3abcBD16989F875707
ERC20 deployed:     0x5FbDB2315678afecb367f032d93F642f64180aa3
...
  Zeto based fungible token with anonymity and encryption
Deploying as upgradeable contracts
ZetoToken deployed: 0x9A9f2CCfdE556A7E9Ff0848998Aa4a0CFD8863AE
ERC20 deployed:     0xA51c1fc2f0D1a1b8494Ed1FE312d7C3a78Ed91C0
...
  Zeto based fungible token with anonymity using nullifiers and encryption
Deploying as upgradeable contracts
ZetoToken deployed: 0x84eA74d481Ee0A5332c457a4d796187F6Ba67fEB
ERC20 deployed:     0xa85233C63b9Ee964Add6F2cffe00Fd84eb32338f

On the other hand, making this change avoids failures in testing against a non-hardhat network, because the same ERC20 contract is used for all test suites, due to ignition layer caching.

Copy link
Contributor

Choose a reason for hiding this comment

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

So if the smart contract accidentally doubled the amount in the transfer logic, this check will throw an error because it knows it's running in Hardhat mode, therefore, gte need to be switched to equal?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure I understand the question @Chengxuan ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the best way to address the concern of missing implementation errors like you pointed out before, without sacrificing public chain testability (where the ERC20 contract is re-used across the test suites due to ignition caching), is querying for the starting balance and ending balance around the Zeto operation. This way we can change back the check to equal.


const tx1 = await erc20.connect(Alice.signer).approve(zeto.target, 100);
await tx1.wait();
Expand Down Expand Up @@ -128,43 +125,52 @@ describe("Zeto based fungible token with anonymity without encryption or nullifi

// Alice checks her ERC20 balance
const balance = await erc20.balanceOf(Alice.ethAddress);
expect(balance).to.equal(80);
});

it("Alice attempting to withdraw spent UTXOs should fail", async function () {
// Alice proposes the output ERC20 tokens
const outputCommitment = newUTXO(90, Alice);

const { inputCommitments, outputCommitments, encodedProof } = await prepareWithdrawProof(Alice, [utxo100, ZERO_UTXO], outputCommitment);

await expect(zeto.connect(Alice.signer).withdraw(10, inputCommitments, outputCommitments[0], encodedProof)).rejectedWith("UTXOAlreadySpent");
expect(balance).to.be.gte(80);
});

it("mint existing unspent UTXOs should fail", async function () {
await expect(doMint(zeto, deployer, [utxo4])).rejectedWith("UTXOAlreadyOwned");
});

it("mint existing spent UTXOs should fail", async function () {
await expect(doMint(zeto, deployer, [utxo1])).rejectedWith("UTXOAlreadySpent");
});

it("transfer non-existing UTXOs should fail", async function () {
const nonExisting1 = newUTXO(10, Alice);
const nonExisting2 = newUTXO(20, Alice, nonExisting1.salt);
await expect(doTransfer(Alice, [nonExisting1, nonExisting2], [nonExisting1, nonExisting2], [Alice, Alice])).rejectedWith("UTXONotMinted");
});

it("transfer spent UTXOs should fail (double spend protection)", async function () {
// create outputs
const utxo5 = newUTXO(25, Bob);
const utxo6 = newUTXO(5, Alice, utxo5.salt);
await expect(doTransfer(Alice, [utxo1, utxo2], [utxo5, utxo6], [Bob, Alice])).rejectedWith("UTXOAlreadySpent")
});
describe('failure cases', function () {
// the following failure cases rely on the hardhat network
// to return the details of the errors. This is not possible
// on non-hardhat networks
if (network.name !== 'hardhat') {
return;
}

it("spend by using the same UTXO as both inputs should fail", async function () {
const utxo5 = newUTXO(20, Alice);
const utxo6 = newUTXO(10, Bob, utxo5.salt);
await expect(doTransfer(Bob, [utxo7, utxo7], [utxo5, utxo6], [Alice, Bob])).rejectedWith(`UTXODuplicate(${utxo7.hash.toString()}`);
it("Alice attempting to withdraw spent UTXOs should fail", async function () {
// Alice proposes the output ERC20 tokens
const outputCommitment = newUTXO(90, Alice);

const { inputCommitments, outputCommitments, encodedProof } = await prepareWithdrawProof(Alice, [utxo100, ZERO_UTXO], outputCommitment);

await expect(zeto.connect(Alice.signer).withdraw(10, inputCommitments, outputCommitments[0], encodedProof)).rejectedWith("UTXOAlreadySpent");
});

it("mint existing unspent UTXOs should fail", async function () {
await expect(doMint(zeto, deployer, [utxo4])).rejectedWith("UTXOAlreadyOwned");
});

it("mint existing spent UTXOs should fail", async function () {
await expect(doMint(zeto, deployer, [utxo1])).rejectedWith("UTXOAlreadySpent");
});

it("transfer non-existing UTXOs should fail", async function () {
const nonExisting1 = newUTXO(10, Alice);
const nonExisting2 = newUTXO(20, Alice, nonExisting1.salt);
await expect(doTransfer(Alice, [nonExisting1, nonExisting2], [nonExisting1, nonExisting2], [Alice, Alice])).rejectedWith("UTXONotMinted");
});

it("transfer spent UTXOs should fail (double spend protection)", async function () {
// create outputs
const utxo5 = newUTXO(25, Bob);
const utxo6 = newUTXO(5, Alice, utxo5.salt);
await expect(doTransfer(Alice, [utxo1, utxo2], [utxo5, utxo6], [Bob, Alice])).rejectedWith("UTXOAlreadySpent")
});

it("spend by using the same UTXO as both inputs should fail", async function () {
const utxo5 = newUTXO(20, Alice);
const utxo6 = newUTXO(10, Bob, utxo5.salt);
await expect(doTransfer(Bob, [utxo7, utxo7], [utxo5, utxo6], [Alice, Bob])).rejectedWith(`UTXODuplicate(${utxo7.hash.toString()}`);
});
});

async function doTransfer(signer: User, inputs: UTXO[], outputs: UTXO[], owners: User[]) {
Expand Down
Loading
Loading