-
Notifications
You must be signed in to change notification settings - Fork 17
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
Ethereum contract #890
Ethereum contract #890
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work!
return PublicKey(resultX, resultY); | ||
} | ||
|
||
function deriveEpsilon(string memory path, address requester) public pure returns (uint256) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function should not be a part of the contract interface.
Let's move all public functions to the top with sign and pk functions on top. That would be convenient for people who want to integrate with this contract.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a few more places where functions should not be public.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted, they are otherwise not easy to test with, so i leave them public in the proof of concept. Will be refactored right after the proof of concept works
return epsilon; | ||
} | ||
|
||
function sign(bytes32 payload, string memory path) external payable returns (bytes32) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is important to have a key version parameter here, even if this logic has not been implemented yet to avoid breaking changes later.
Should we use structs in function APIs? I am not sure what the best practices in Solidity are.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will add a key version. Struct can be used, costing a bit more gas.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added
** - scalar decomposition through endomorphism | ||
** This library does not check whether the inserted points belong to the curve | ||
** `isOnCurve` function should be used by the library user to check the aforementioned statement. | ||
** @author Witnet Foundation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this and other tools available as a library?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is, the other secp256k1 one is not. We probably need our custom related optimization on this file, trim it, and gas optimize it further
const { buildModule } = require("@nomicfoundation/hardhat-ignition/modules"); | ||
|
||
const DEFAULT_PUBLIC_KEY = { | ||
x: "0xfc115813e59a914d7566a4b1d4263048faf6b6dab6893c4d65d39fb123da5651", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it something specific?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, for testing
@@ -0,0 +1,81 @@ | |||
const hre = require("hardhat"); | |||
|
|||
async function main() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what we should have in the end. JS tests may be hard to integrate into our current infra and harder to support.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did more research, currently hardhat is still most common option in eth and most evm compatible chains. This file and responder.js is temporary one that will be dropped after integrate with mpc node
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, we should use what is the industry standard
@@ -0,0 +1,53 @@ | |||
const hre = require("hardhat"); | |||
|
|||
async function main() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want to keep this after you add a real indexer to the main system?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it is reference client before integration with node is done
@volovyks ptal. Addressed most of your points. Other not addressed points are valid too, will update them after finishing the indexer & gateway parts |
First part of #858 . The interface can be found in ChainSignatures.test.js. The precomputed value is taken from rust contract / test output in
bo/eth-reference
branchRefer to README in this PR to see how this part works. scripts mentioned in README is for prototype and a reference impl, they will be removed when actual impl in mpc node is finished