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

Update Package.swift #819

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

MaksymVereshchaka
Copy link

Summary of Changes

Fixes # (if applicable - add the number of issue this PR addresses)

Test Data or Screenshots

By submitting this pull request, you are confirming the following:
  • I have reviewed the Contribution Guidelines.
  • I have performed a self-review of my own code.
  • I have updated my repository to match the develop branch.
  • I have included test data or screenshots that prove my fix is effective or that my feature works.
  • I have checked that all tests work and swiftlint is not throwing any errors/warnings.

@JeneaVranceanu
Copy link
Collaborator

@MaksymVereshchaka
I guess you had issues building your project.
If that was the case, could you please share a list of dependencies of your project?

@JeneaVranceanu JeneaVranceanu self-requested a review May 3, 2023 19:36
@MaksymVereshchaka
Copy link
Author

@JeneaVranceanu
I use BitcoinKit (https://github.com/horizontalsystems/BitcoinKit.Swift). It has depends from HdHodler, which the authors rewrote using secp256k1 from https://github.com/Boilertalk/secp256k1.swift to https://github.com/GigaBitcoin/secp256k1.swift. When I try to connect now Web3Swift I get error secp256k1 and secp256k1.swift and need to use moduleAliases as shown here: https://github.com/apple/swift-evolution/blob/main/proposals/0339-module-aliasing-for-disambiguation.md . I tried using moduleAliases but found a better solution, which I have attached here.

@JeneaVranceanu
Copy link
Collaborator

Got it. I had the same issue with one of the versions of WalletConnectV2.
I'll have to take some time to consider if this is the solution we want to use. I do not see at the moment anything wrong about but I have to investigate further.

@yaroslavyaroslav
Copy link
Collaborator

yaroslavyaroslav commented May 4, 2023

I'll have to take some time to consider if this is the solution we want to use. I do not see at the moment anything wrong about but I have to investigate further.

Do I see it right, that the only significant change here is that the secp dependency is made external one?

If so, here's what I have to say:

  1. Kinda this situations is going on all the way with this dependency in pods. It's pointing to some forever forgotten pod with that.
  2. I suggest to push it further, and if we replace this dependency in Package let's drop its code along with that.

Also I remember that we had discuss this thing a while ago in Discord, like whether should we replace it with the bitcoin implementation, and we decided to yes, but someday later

@JeneaVranceanu
Copy link
Collaborator

JeneaVranceanu commented May 4, 2023

Do I see it right, that the only significant change here is that the secp dependency is made external one?

Yes, that's the change.

To say something to your points:

  1. This is a dependency that could reach a point where there's nothing really too much to update. There will be bugs, but no more new features. web3swift has the secp256k1 module since Apr 30th 2019 and there were no updates made to the code in that module;
  2. Of course, we will have to drop secp256k1 code from the code base if the dependency is replaced, but before we will decide to do so we have to make sure that using this new dependency won't create the same "duplicated module" issue when web3swift will be combined with something else.

Additionally, there is always a security risk when it comes to the code that is responsible for key-pair generation, signing and encryption overall. It's best to stick to some fixed version IMHO.

@janndriessen
Copy link
Collaborator

Just reporting that I also have the same issue now with another library. And to follow the conversation here.

@yaroslavyaroslav
Copy link
Collaborator

Additionally, there is always a security risk when it comes to the code that is responsible for key-pair generation, signing and encryption overall. It's best to stick to some fixed version IMHO.

Just to say, there's the opposite security risks exists, when you reimplement any crypto by yourself instead of using some popular bullet proof implementation.

Anyway I agree with you in general — there should be no rush in this in anyway.

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.

None yet

4 participants