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

Ethereum contract #890

Merged
merged 17 commits into from
Nov 11, 2024
Merged

Ethereum contract #890

merged 17 commits into from
Nov 11, 2024

Conversation

ailisp
Copy link
Member

@ailisp ailisp commented Oct 14, 2024

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 branch

Refer 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

@ailisp ailisp marked this pull request as ready for review October 31, 2024 04:38
@ailisp ailisp changed the title wip eth contract Ethereum contract Oct 31, 2024
volovyks
volovyks previously approved these changes Nov 6, 2024
Copy link
Collaborator

@volovyks volovyks left a 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) {
Copy link
Collaborator

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.

Copy link
Collaborator

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.

Copy link
Member Author

@ailisp ailisp Nov 8, 2024

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) {
Copy link
Collaborator

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.

Copy link
Member Author

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.

Copy link
Member Author

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
Copy link
Collaborator

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?

Copy link
Member Author

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",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it something specific?

Copy link
Member Author

Choose a reason for hiding this comment

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

no, for testing

chain-signatures/contract-eth/test/ChainSignatures.test.js Outdated Show resolved Hide resolved
@@ -0,0 +1,81 @@
const hre = require("hardhat");

async function main() {
Copy link
Collaborator

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.

Copy link
Member Author

@ailisp ailisp Nov 8, 2024

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

Copy link
Collaborator

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() {
Copy link
Collaborator

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?

Copy link
Member Author

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

@ailisp
Copy link
Member Author

ailisp commented Nov 8, 2024

@volovyks ptal. Addressed most of your points. Other not addressed points are valid too, will update them after finishing the indexer & gateway parts

@ailisp ailisp merged commit b69ce2f into develop Nov 11, 2024
1 check passed
@ailisp ailisp deleted the eth branch November 11, 2024 04:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants