Zodiac test2 #4
Zodiac test2 #4
Conversation
restyled layout according to layout guidelines
First test compile passed.
Added comments regarding the TODO for portions of code that need to be adjusted based on review and feedback.
Created grant Minter Role after verifying signature of msg.sender as signer. //TODO Add clean comments to every line. //TODO Installed prettier but it seems to be clashing with Solidity extension.
Replaced AccessControl with Ownable.
InstalledOZ Contracts versions now match according to dependency in package.json.
Added files in .gitignore.
//TODO Add in test case for msg.sender being able to obtain minter role //TODO Add in test case for minter role being able to mint //TODO Add in const for domain
//TODO: Fix weird error requesting for different compiler version despite 0.8.18 successfully compiling.
Added RPC/key dev values for deployment in hardhat.config.ts
… and viem test case //TODO Fix unused types and declare message to be signed by add2.
| * @notice interfaced from Shards.sol to obtain address, token Id, amount owned and stored data | ||
| * for future use | ||
| */ | ||
| interface IShards { |
There was a problem hiding this comment.
nit: put interfaces in an external file
| // Shards and domain separator constant is initialised | ||
| // using name, version and Arbitrum Goerli chainID. | ||
| constructor() { | ||
| if (msg.sender != owner()) { |
There was a problem hiding this comment.
i don't think this check is necessary
| //Mapping SHARD_ID to the individual enclaves | ||
| uint256[] public SHARD_ID = [2, 3, 4, 5, 6]; | ||
| string[] public ENCLAVE = [ | ||
| "", |
There was a problem hiding this comment.
what's the empty string for?
|
|
||
| //for loop checks if the Enclave name matches the Shard ID | ||
| function establishMapping() public { | ||
| // Establish the mapping between SHARD_ID and ENCLAVE |
There was a problem hiding this comment.
gas: uint256 _length = SHARD_ID.length;. and then use _length in the for loop
| //for loop checks if the Enclave name matches the Shard ID | ||
| function establishMapping() public { | ||
| // Establish the mapping between SHARD_ID and ENCLAVE | ||
| for (uint256 i = 2; i < SHARD_ID.length; i++) { |
There was a problem hiding this comment.
this for loop doesn't map all values. starting from 2, you miss chaosEnclave.
uint256 _length = SHARD_ID.length;
for(uint i=0; i<_length) {
shardToEnclave[SHARD_ID[i]] = ENCLAVE[i+1];
unchecked {
++i;
}
}
You could also do this in the constructor as it's a one-time thing
| if (_shardIndex < SHARD_ID.length || _shardIndex > SHARD_ID.length) { | ||
| revert InvalidMint(msg.sender); | ||
| } | ||
| SHARDS.partnerMint(msg.sender, SHARD_ID[_shardIndex], 1, ""); |
There was a problem hiding this comment.
you have not set SHARDS to an address
| // original deployer is granted the default admin role | ||
| // Shards and domain separator constant is initialised | ||
| // using name, version and Arbitrum Goerli chainID. | ||
| constructor() { |
There was a problem hiding this comment.
you have not set SHARDS or RELIC
| if (err != ECDSA.RecoverError.NoError) { | ||
| revert InvalidSignature(request.account); | ||
| } | ||
| // Check for error if deadline is expired |
There was a problem hiding this comment.
nit: I would put checks at beginning of function
| // Defining the CONSTANTS which are of bytes32 type | ||
| bytes32 constant MINTREQUEST_TYPEHASH = | ||
| keccak256("MintRequest(address signer,uint256 deadline,uint256 nonce)"); | ||
| bytes32 constant EIP712DOMAIN_TYPEHASH = |
There was a problem hiding this comment.
add address verifyingContract
bytes32 private constant EIP712DOMAIN_TYPEHASH =
keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)");
| keccak256(bytes(_input.name)), | ||
| keccak256(bytes(_input.version)), | ||
| _input.chainId | ||
| ) |
There was a problem hiding this comment.
add verifyingAddress address(this). see link from previous comment
| @@ -0,0 +1,95 @@ | |||
| const { expect } = require("chai"); | |||
| it("Check if mapping of enclave to Shard Id is correct", async function () { | ||
| await pathOfTheTemplarShard.establishMapping(); | ||
|
|
||
| for (let i = 2; i < SHARD_ID.length; i++) { |
There was a problem hiding this comment.
i mentioned this in contract review, so update test
| expect(await pathOfTheTemplarShard.minters(account)).to.equal(value); | ||
|
|
||
| }); | ||
|
|
There was a problem hiding this comment.
add tests that fail for owner guarded functions when called by nonowner
| const signature = await add2._signTypedData(data.domain, data.types, data.message); | ||
|
|
||
| await pathOfTheTemplarShard.setMinter(await add2.getAddress(), true); | ||
| await pathOfTheTemplarShard.connect(add3).mintShard(MintRequest, signature, 2); |
There was a problem hiding this comment.
why connect add3 and not add2?
| pathOfTheTemplarShard, | ||
| } = await loadFixture(setup)); | ||
| }); | ||
|
|
There was a problem hiding this comment.
some tests are missing. check test coverage and add tests for view functions too
|
|
||
| }); | ||
|
|
||
| it("Check if a signer that is not whitelisted minter produces an invalid signature", async function () { |
There was a problem hiding this comment.
get signed typed data of address which can't mint and expect custom error from contract after calling with:
- invalid signature
- can't mint address
3 participants
Latest branch for fixing unit test errors working with yarn to be consistent with Temple monorepo