web3swift icon indicating copy to clipboard operation
web3swift copied to clipboard

Update Package.swift

Open MaksymVereshchaka opened this issue 1 year ago • 7 comments

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.

MaksymVereshchaka avatar May 03 '23 18:05 MaksymVereshchaka

@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 avatar May 03 '23 18:05 JeneaVranceanu

@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.

MaksymVereshchaka avatar May 04 '23 06:05 MaksymVereshchaka

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.

JeneaVranceanu avatar May 04 '23 07:05 JeneaVranceanu

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

yaroslavyaroslav avatar May 04 '23 11:05 yaroslavyaroslav

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.

JeneaVranceanu avatar May 04 '23 15:05 JeneaVranceanu

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

janndriessen avatar May 05 '23 10:05 janndriessen

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.

yaroslavyaroslav avatar May 05 '23 11:05 yaroslavyaroslav