This repository has been archived by the owner on Oct 14, 2024. It is now read-only.

Starknetv0.11 Ready for review #21

Merged

FawadHa1der merged 23 commits into main from starknetv0.11

Apr 7, 2023

Contributor

FawadHa1der commented Mar 31, 2023

No description provided.

FawadHa1der added 4 commits

March 22, 2023 10:15


          poseidon test

a8c077a


          initial migration to use the new commitment scheme

9a6ec46


          update the abis and move to new deployed l1resolver/verifier contract

812f5db


          unit test updates

fdc9ece

FawadHa1der marked this pull request as ready for review

April 5, 2023 16:41

FawadHa1der requested review from prix0007 and JorikSchellekens

April 5, 2023 16:54

FawadHa1der changed the title ~~Starknetv0.11 WIP~~ Starknetv0.11 Ready for review

FawadHa1der added 9 commits

April 5, 2023 14:43


          deploying poseidon only for local deployment

c1eca9c


          update core contract root used for testing

5a98fb6


          move the gateway abi interface to l1resolver filde

75a7139


          remove docker compose file

f39e618


          update the test work area

21a40b0


          update the abis

e2b70e9


          readme update

79157f6


          upgrading l2resolver from cairo0 to cairo1

6a29cf1


          readme update

68c55e6

achab reviewed

View reviewed changes

README.md Outdated

Comment on lines 22 to 26

+              SNL1ResolverStub.sol inherits from SNStateProofVerifier.sol and has been deployed to Goerli at 0xBB49c34D4d92aC3207d589657fAC14186a470116
+              On goerli Pedersen hash contract is already deployed at 0x1a1eB562D2caB99959352E40a03B52C00ba7a5b1
+              Poseidon3(starkewares version) contracts EVM code has ben generated from the following repo and deployed on Goerli at 0x84d43a8cbEbF4F43863f399c34c06fC109c957a4.

achab Apr 6, 2023

could we add links to goerli etherscan to these contracts' addresses?

Contributor Author

FawadHa1der Apr 6, 2023

done

achab reviewed

View reviewed changes

README.md Outdated


		SNL1ResolverStub.sol inherits from SNStateProofVerifier.sol and has been deployed to Goerli at 0xBB49c34D4d92aC3207d589657fAC14186a470116

		On goerli Pedersen hash contract is already deployed at 0x1a1eB562D2caB99959352E40a03B52C00ba7a5b1

achab Apr 6, 2023

could we verify this contract?

Contributor Author

FawadHa1der Apr 6, 2023 •

edited

Loading

Yes 0xBB49c34D4d92aC3207d589657fAC14186a470116 has been verified but this will change due to the changes as part of this review

achab reviewed

View reviewed changes

README.md Outdated


		On goerli Pedersen hash contract is already deployed at 0x1a1eB562D2caB99959352E40a03B52C00ba7a5b1

		Poseidon3(starkewares version) contracts EVM code has ben generated from the following repo and deployed on Goerli at 0x84d43a8cbEbF4F43863f399c34c06fC109c957a4.

achab Apr 6, 2023

same here: could we verify this contract?

Contributor Author

FawadHa1der Apr 6, 2023

Pedersen contract is not so simple to verify, I have created an issue here -> #22

achab reviewed

View reviewed changes

README.md Outdated

               ## L2 Resolver
-              L2 resolver is a git subtree of https://github.com/starknet-id/ens_resolver. This has been deployed to 0x7412b9155cdb517c5d24e1c80f4af96f31f221151aab9a9a1b67f380a349ea on goerli
+              This has been upgraded to cairo1 and deployed to 0x7412b9155cdb517c5d24e1c80f4af96f31f221151aab9a9a1b67f380a349ea on goerli

achab Apr 6, 2023

same here, it would be good to verify the contract here as well. and add a link to voyager

Contributor Author

FawadHa1der Apr 6, 2023

I did try, cairo1 tooling is a work in progress I think, will try again -> #23

achab reviewed

View reviewed changes

contracts/contracts/SNStateProofVerifier.sol Outdated

    
                  // this is the hades permutation function. TODO update the name when goerli eth is not so expensive

                  function poseidon(

                      uint256[3] calldata input

                  ) external view returns (uint256[3] calldata);

achab Apr 6, 2023

I'm a bit surprised by calldata here. This is only available for function arguments of external functions.

Contributor Author

FawadHa1der Apr 6, 2023

I am not so sure about that, you can use it for contract to contract calls and it works fine, its a little bit cheaper I think(but could be wrong) and one should be aware of the downsides(immutability etc etc). But you are right that that is not the convention and probably not worth the confusion it will cause. I have updated it to memory. Interesting question in general though. Happy to be corrected on this understanding

achab reviewed

View reviewed changes

contracts/contracts/SNStateProofVerifier.sol Outdated

Comment on lines 133 to 137

+                      for (uint256 i = 0; i < elems.length / 2; i++) {
+                          state[0] = (state[0] + elems[2 * i]) % BIG_PRIME;
+                          state[1] = (state[1] + elems[2 * i + 1]) % BIG_PRIME;
+                          state = poseidon.poseidon(state);
+                      }

achab Apr 6, 2023

alternative to avoid division and multiplication:

for (uint256 i = 0; i < elems.length; i += 2) {
    state[0] = (state[0] + elems[i]) % BIG_PRIME;
    state[1] = (state[1] + elems[i + 1]) % BIG_PRIME;
    state = poseidon.poseidon(state);
}

Contributor Author

FawadHa1der Apr 6, 2023 •

edited

Loading

but with (i < elems.length -1) but great suggestion, thanks Massil

achab reviewed

View reviewed changes

contracts/contracts/SNStateProofVerifier.sol

+                  function poseidonHashMany(
+                      uint256[] memory elems
+                  ) public view returns (uint256) {
+                      uint256[3] memory state = [uint256(0), uint256(0), uint256(0)];

achab Apr 6, 2023

maybe I'm missing something, but I feel that the code here never updates state[2].

achab Apr 6, 2023

oh, it's probably updated by the function poseidon.poseidon(state), right?

Contributor Author

FawadHa1der Apr 6, 2023

Yes that is correct

achab reviewed

View reviewed changes

contracts/contracts/SNStateProofVerifier.sol Outdated

+                          );
+                          // since we have already verified the first element in the proof array correctly hashes up to the state commitment, we can assume the hash of first element in the proof array is correct.
+                          expectedHash = calculatedContractStateRoot;
+                      }
                       require(

achab Apr 6, 2023

we can move this check before the if statement

Contributor Author

FawadHa1der Apr 6, 2023

done

achab reviewed

View reviewed changes

contracts/contracts/SNStateProofVerifier.sol Outdated

Comment on lines 293 to 296

+                          require(
+                              proofArray.length > 0,
+                              "proofs must have atleast one element!"
+                          );

achab Apr 6, 2023

if we move the same check below above the if statement, we can remove this one

Contributor Author

FawadHa1der Apr 6, 2023

done

achab reviewed

View reviewed changes

contracts/contracts/StarknetCoreContractStub.sol Outdated

@@ @@ -7,13 +7,13 @@ contract StarknetCoreContractStub { @@
                       */
                   function stateRoot() external view returns (uint256) {
                       return
-                          3321080970052968843805320357911538392446265524314601848332135665594593955343;
+                          2793869633745137896926484933765493995916819896677591814984705621839603171245;
                   }
                   /**
                       Returns the current block number.

achab Apr 6, 2023

the comment is not accurate. what does correspond to the block number 789146?

Contributor Author

FawadHa1der Apr 6, 2023

updated the comment

achab reviewed

View reviewed changes

contracts/contracts/StarknetCoreContractStub.sol Outdated

@@ @@ -7,13 +7,13 @@ contract StarknetCoreContractStub { @@
                       */
                   function stateRoot() external view returns (uint256) {
                       return
-                          3321080970052968843805320357911538392446265524314601848332135665594593955343;
+                          2793869633745137896926484933765493995916819896677591814984705621839603171245;

achab Apr 6, 2023

what the meaning of this value?

Contributor Author

FawadHa1der Apr 6, 2023

There is a comment at the top of the file mentioning its only to be used in tests. I have moved it into mocks folder to make it more explicit. Its mock contract used only unit tests

achab reviewed

View reviewed changes

contracts/scripts/deploy.ts Outdated

                         ["https://starknetens.ue.r.appspot.com/{sender}/{data}.json"], // per https://eips.ethereum.org/EIPS/eip-3668  the sender and data populated by the client library like ethers.js with data returned by the CCIP enabled contract via revert
-                        '0x7412b9155cdb517c5d24e1c80f4af96f31f221151aab9a9a1b67f380a349ea3'
+                        '0x7412b9155cdb517c5d24e1c80f4af96f31f221151aab9a9a1b67f380a349ea3' // L2 resolver contract address on starknet

achab Apr 6, 2023

we can use the constant L2_RESOLVER_ADDRESS here

achab reviewed

View reviewed changes

contracts/scripts/deploy.ts Outdated

                     const proofProxy = await upgrades.deployProxy(
                       SNResolverStub,
-                      [pedersenHash.address, starknetCoreContractStub.address, ["https://localhost:8080/{sender}/{data}.json"], '0x7412b9155cdb517c5d24e1c80f4af96f31f221151aab9a9a1b67f380a349ea3'],
+                      [pedersenHash.address, poseidon3.address
+                        , starknetCoreContractStub.address, ["https://localhost:8080/{sender}/{data}.json"], '0x7412b9155cdb517c5d24e1c80f4af96f31f221151aab9a9a1b67f380a349ea3'],

achab Apr 6, 2023

we can use the constant L2_RESOLVER_ADDRESS here

Contributor Author

FawadHa1der Apr 6, 2023 •

edited

Loading

This applies to all L2_RESOLVER_ADDRESS mentions below as well. They cannot all point to the same const var because frontend, contracts, gateway, client test are all independent apps with their package.json and also they happen to be the same values right now but they can be different especially in tests. Also one might want to deploy different resolvers but not yet ready to have the change reflected in the frontend app. Though I have added constants in deploy.ts file just to make it more explicit.

achab reviewed

View reviewed changes

contracts/test/stateverifier.test.ts

                     const proofProxy = await upgrades.deployProxy(
                       SNResolverStub,
-                      [pedersenHash.address, starknetCoreContractStub.address, ["https://localhost:9545/{sender}/{data}.json"], '0x7412b9155cdb517c5d24e1c80f4af96f31f221151aab9a9a1b67f380a349ea3'],
+                      [pedersenHash.address, poseidon3.address, starknetCoreContractStub.address, ["https://localhost:9545/{sender}/{data}.json"], '0x7412b9155cdb517c5d24e1c80f4af96f31f221151aab9a9a1b67f380a349ea3'],

achab Apr 6, 2023

same here, we can use L2_RESOLVER_ADDRESS

Contributor Author

FawadHa1der Apr 6, 2023

see comment above about L2_RESOLVER_ADDRESS

achab reviewed

View reviewed changes

contracts/test/stateverifier.test.ts

                 it("it should verify the sampleproof1.json", async function () {
                   var jsonFile = "./sampleProof1.json";
                   const contractAddress =
-                    "0x6fbd460228d843b7fbef670ff15607bf72e19fa94de21e29811ada167b4ca39";
+                    "0x7412b9155cdb517c5d24e1c80f4af96f31f221151aab9a9a1b67f380a349ea3";

achab Apr 6, 2023

L2_RESOLVER_ADDRESS

Contributor Author

FawadHa1der Apr 6, 2023

see comment above about L2_RESOLVER_ADDRESS

achab reviewed

View reviewed changes

contracts/test/stateverifier.test.ts

                 it("it should verify the sampleproof1.json", async function () {
                   var jsonFile = "./sampleProof1.json";
                   const contractAddress =
-                    "0x6fbd460228d843b7fbef670ff15607bf72e19fa94de21e29811ada167b4ca39";
+                    "0x7412b9155cdb517c5d24e1c80f4af96f31f221151aab9a9a1b67f380a349ea3";

achab Apr 6, 2023

L2_RESOLVER_ADDRESS

Contributor Author

FawadHa1der Apr 6, 2023

no, this can be any contract, doesn't have to be a L2_RESOLVER_ADDRESS. these are just verifying proofs for a storage value for any contract.

achab reviewed

View reviewed changes

contracts/scripts/deploy.ts Outdated

Comment on lines 75 to 77

+                        "0x1a1eB562D2caB99959352E40a03B52C00ba7a5b1", // pedersen already deployed on goerli (goerli eth is expensive, cant do reployments all the time)
+                        "0x84d43a8cbEbF4F43863f399c34c06fC109c957a4", // poseidon already deployed on goerli (goerli eth is expensive, cant do reployments all the time)
+                        "0xde29d060D45901Fb19ED6C6e959EB22d8626708e", // starknetcore contract address on goerli

achab Apr 6, 2023

we can create constants for these

Contributor Author

FawadHa1der Apr 6, 2023

done

achab reviewed

View reviewed changes

contracts/test/stateverifier.test.ts

@@ @@ -134,13 +152,20 @@ describe("Verify", function () { @@
                   expect(resultFromStarknetVerifier.toString()).to.be.eq(result[0]);
                 });
+                it(" test for a poseidon array", async function () {
+                  const result = await l1resolverStub.poseidonHashMany(["28355430774503553497671514844211693180464", "0x34894aedf9548524f9e5bb189472d25abe3c38befd577c90886a7c519e5eee4", "0x70c5acad61a421be9c2945b921e263f2699c668f20d31c90660e91b32ea99de"]);

achab Apr 6, 2023

we could create a constant FELT_FOR_STARKNET_STATE_V0 for 28355430774503553497671514844211693180464

achab Apr 6, 2023

and for other numbers as well

Contributor Author

FawadHa1der Apr 6, 2023

These any be any random values, just checking poseidonHashMany works for any random set of values, I have added a few more tests

achab reviewed

View reviewed changes

contracts/test/testpy.py

Comment on lines +21 to +23

+xca8ac56bd313f8355dc40776427409f119924fe9067320fff8fd8765b766df, 0x7fb20da4f329a630f4bdfb75afae4ec93140da3a2527986c2cb7ee7361b9dea) ))
+              print ('STARKNET_STATE_V0 to felt ',short_string_to_felt('STARKNET_STATE_V0') )
+              print ("state commitment poseidon_hash_many ", hex(poseidon_hash_many([28355430774503553497671514844211693180464, 0x34894aedf9548524f9e5bb189472d25abe3c38befd577c90886a7c519e5eee4, 0x70c5acad61a421be9c2945b921e263f2699c668f20d31c90660e91b32ea99de])))

achab Apr 6, 2023

we could create constants for these numbers

Contributor Author

FawadHa1der Apr 6, 2023

This is just a rough work area to compare values, not used as part of any script, there is a explicit comment at the top of the file. Probably should not be checked in, I only checked it in so that anybody can easily play/compare values with starkware's python implementation

FawadHa1der added 9 commits

April 6, 2023 17:36


          move StarknetCoreContractStub.sol to mocks folder

2ed31fd


          move StarknetCoreContractStub.sol to mocks folder

772fa14


          performance improvment for poseidon array, calldata to memory functio…

48e462d

…n parameter, remoove redundant proof array size check


          Add more comments to make explicit the puprose of the starknetcoercon…

e68f642

…tractstub


          adding more poseidon array tests


          adding goerli deployment constants

855a361


          readme updates with links to etherscan and voyager

2a66654


          updated deployment address for new verifier/resolver on goerli


          updated deployment address for new verifier/resolver on goerli

d2bf5f0

prix0007 approved these changes

View reviewed changes

Collaborator

prix0007 left a comment

LGTM

contracts/.DS_Store Outdated

Collaborator

prix0007 Apr 5, 2023

remove this file as this is not required.


          update the new abis

f58aa9f

achab approved these changes

View reviewed changes

FawadHa1der merged commit 4945f5d into main

Contributor Author

FawadHa1der commented Apr 7, 2023

Thanks Prince and Massil for the review

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet