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

Pocket Integration #127

Open
wants to merge 27 commits into
base: master
Choose a base branch
from
Open

Conversation

pabelnl
Copy link

@pabelnl pabelnl commented Oct 14, 2019

Includes:

  • PocketNetworkClient: Public class that conforms to the NetworkClient protocol, adding the necessary steps and information to interact with the Pocket Network.

  • Carthage dependency: Added pocket-swift-core-carthage to the carfile, necessary to use all the core functionalities for Pocket.

  • Updated Project: Added necessary frameworks to the carthage script section and to the test target copy files to properly run the tests.

@keefertaylor keefertaylor self-requested a review October 16, 2019 00:22
@keefertaylor
Copy link
Owner

Can you provide the commands you're using to build? Running integration tests in Xcode gives me:

The bundle “IntegrationTests” couldn’t be loaded because it is damaged or missing necessary resources. Try reinstalling the bundle.
2019-10-15 20:41:32.976364-0400 xctest[28938:2344211] (dlopen_preflight(/Users/keefertaylor/Library/Developer/Xcode/DerivedData/TezosKit-azhwkmvjudwqolgugmsphxlkimqx/Build/Products/Debug-iphonesimulator/IntegrationTests.xctest/IntegrationTests): Library not loaded: @rpath/RNCryptor.framework/RNCryptor
  Referenced from: /Users/keefertaylor/Library/Developer/Xcode/DerivedData/TezosKit-azhwkmvjudwqolgugmsphxlkimqx/Build/Products/Debug-iphonesimulator/TezosKit.framework/Frameworks/PocketSwift.framework/PocketSwift
  Reason: no suitable image found.  Did find:
	/Users/keefertaylor/Library/Developer/Xcode/DerivedData/TezosKit-azhwkmvjudwqolgugmsphxlkimqx/Build/Products/Debug-iphonesimulator/TezosKit.framework/Frameworks/PocketSwift.framework/Frameworks/RNCryptor.framework/RNCryptor: no matching architecture in universal wrapper)

I also have some failures in PocketNetwork unit tests.

Copy link
Owner

@keefertaylor keefertaylor left a comment

Choose a reason for hiding this comment

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

Thank you for taking the time to contribute to TezosKit. This PR overall looks great.

Here are a few notes that I think will help this integration remain easy to maintain and make it easy for clients to use. I don't feel incredibly strongly about these things, and I'm happy to take this PR as is if you'd rather not implement.


If you'd like, I'd be supportive of you adding an extension to TezosNodeClient to automatically make a NodeClient with the Pocket network attached. You could place in /Pocket/TezosNodeClient+Pocket and implement with:

extension PocketNetworkClient {
  public static pocketNetworkNodeClient() -> PocketNetworkClient {
    let pocketNEtworkClient = PocketNetworkClient(...)
    return new TezosNodeClient(networkClient: pocketNetworkClient, ....) 
  }
}

(You could also use a convenience initializer if you prefer to the static factory method.


If you'd like to have integration tests, you can feel free to add them. I run integration tests manually on merge.

TezosKit/Client/PocketNetworkClient.swift Show resolved Hide resolved
Cartfile Show resolved Hide resolved
TezosKit/Client/PocketNetworkClient.swift Outdated Show resolved Hide resolved
TezosKit/Client/PocketNetworkClient.swift Outdated Show resolved Hide resolved
Tests/TezosKit/PocketNetworkClientTest.swift Outdated Show resolved Hide resolved
@keefertaylor
Copy link
Owner

Here are the commands and resulting log files:

Unit Tests:

$ set -o pipefail && xcodebuild test -scheme TezosKit -destination 'platform=iOS Simulator,name=iPhone 8,OS=13.0' ONLY_ACTIVE_ARCH=YES | xcpretty

Output: https://gist.github.com/keefertaylor/717bbea39a9e788e049da0397019953f


Integration Tests:

$ set -o pipefail && xcodebuild test -scheme IntegrationTests -destination 'platform=iOS Simulator,name=iPhone 8,OS=13.0' ONLY_ACTIVE_ARCH=YES | xcpretty

Output: https://gist.github.com/keefertaylor/c4db31c1b82b535476064afd08ed491c

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