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

feat: mpc swift provider #15

Closed
wants to merge 7 commits into from
Closed

feat: mpc swift provider #15

wants to merge 7 commits into from

Conversation

ieow
Copy link
Contributor

@ieow ieow commented May 20, 2024

No description provided.

@metalurgical
Copy link
Contributor

Isn't this a rehash of #8 ?

Copy link
Contributor

@metalurgical metalurgical left a comment

Choose a reason for hiding this comment

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

Please see the comments on #8. Many of the same comments apply here.



public func sign(message: Data) throws -> Data {
return try self.signer.sign(message: message)
Copy link
Contributor

@metalurgical metalurgical May 20, 2024

Choose a reason for hiding this comment

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

I disagree with this.

You're pushing the tss-client from core kit into the dependencies here, where it is already a dependency. This is illustrated with all the extra deps in the tests.

What we should be doing instead in my opinion if you want to do it like this:

  1. Put protocol EvmSigner into it's own package, with basic mock for a test
  2. Import that package here and extend the classes here to conform additionally to EvmSigner
  3. Build the rest of the relevant signers, plus the tests specific to the signers, either here or in their own packages. Import the evm signer package here.
  4. Import the EVMSigner protocol package and relevant signer packages into core kit.
  5. Build tests that integrate core kit and the relevant signers, in core kit.

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.

2 participants