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

Remove Promises #515

Merged
merged 46 commits into from
Apr 27, 2022
Merged

Conversation

pharms-eth
Copy link
Collaborator

@pharms-eth pharms-eth commented Apr 7, 2022

This PR removes the Promises dependency and replaces it with Swift Concurrency (async/await)
Due to the nature of this PR it is breaking to the entire code base
my approach in this PR was to minimize other changes and focus on the Promises changes

[web3swiftEventloopTests.swift] has been disabled. It will require a full rewrite by someone

@pharms-eth pharms-eth marked this pull request as draft April 7, 2022 06:37
@pharms-eth pharms-eth marked this pull request as ready for review April 13, 2022 06:29
@pharms-eth pharms-eth closed this Apr 14, 2022
@pharms-eth pharms-eth reopened this Apr 14, 2022
@mloit
Copy link
Contributor

mloit commented Apr 15, 2022

Now that 2.6.0 is out, please update this so it can be merged in as part of 3.0.

@mloit
Copy link
Contributor

mloit commented Apr 17, 2022

the Carthage Project still cannot find some frameworks on my local machine but are libraries still dependencies for this project

For Carthage to work you need to have the Xcode project up to date. and then run the following from the command line to resolve the dependancies:
carthage checkout

and then build them:
carthage build --no-use-binaries --platform iOS Simulator --use-xcframeworks

note you can set the platform to other targets if you want (macOS, tvOS....) The above is recommended as that is what the automated tests basically use.

@yaroslavyaroslav
Copy link
Collaborator

let web3 = try await Web3.new(URL(string: "http://127.0.0.1:8545")!) let allAddresses = try await web3.eth.getAccounts()

is where they fail for me due to not having your test node setup. Can we put on the roadmap testing the Swift code and not the server response? ie Swift Best Practices are to not do network calls and instead mock or stub or swizzle

Please take a look at the ci/cd pipeline for both Carthage and SPM building in ./.github/workflows/ci.yml, there's provied whole pipeline of building testing from scratch.

Also, please take a look at the Building from source chapter in readme.md.

Package.swift Outdated Show resolved Hide resolved
iOS 15, macOS 12 use the new data(for:
older version fall back on a older implementation
@pharms-eth
Copy link
Collaborator Author

added support to iOS 13 and macOS 10.15

@pharms-eth
Copy link
Collaborator Author

I have updated the Carthage configurations however it is still giving errors

@yaroslavyaroslav yaroslavyaroslav mentioned this pull request Apr 24, 2022
@yaroslavyaroslav
Copy link
Collaborator

@pharms-eth github actions checks wouldn't pass on current ci/cd config you have. Please pull unstable branch to your to get new one, which is updated to work with your merge.

@@ -0,0 +1,7 @@
<?xml version="1.0" encoding="UTF-8"?>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please delete this file

@@ -1522,7 +1512,7 @@
GCC_WARN_UNUSED_VARIABLE = YES;
INFOPLIST_FILE = web3swift.xcodeproj/Info.plist;
IPHONEOS_DEPLOYMENT_TARGET = 12.0;
MACOSX_DEPLOYMENT_TARGET = 10.11;
MACOSX_DEPLOYMENT_TARGET = 12.0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please change it to 10.15 (through Carthage project settings)

@@ -1586,7 +1576,7 @@
GCC_WARN_UNUSED_VARIABLE = YES;
INFOPLIST_FILE = web3swift.xcodeproj/Info.plist;
IPHONEOS_DEPLOYMENT_TARGET = 12.0;
MACOSX_DEPLOYMENT_TARGET = 10.11;
MACOSX_DEPLOYMENT_TARGET = 12.0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please change it to 10.15 (through Carthage project settings)

LD_RUNPATH_SEARCH_PATHS = (
"$(inherited)",
"@executable_path/../Frameworks",
"@loader_path/Frameworks",
);
MACOSX_DEPLOYMENT_TARGET = 10.11;
MACOSX_DEPLOYMENT_TARGET = 12.0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please change it to 10.15 (through Carthage project settings)

LD_RUNPATH_SEARCH_PATHS = (
"$(inherited)",
"@executable_path/../Frameworks",
"@loader_path/Frameworks",
);
MACOSX_DEPLOYMENT_TARGET = 10.11;
MACOSX_DEPLOYMENT_TARGET = 12.0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please change it to 10.15 (through Carthage project settings)

LD_RUNPATH_SEARCH_PATHS = (
"$(inherited)",
"@executable_path/../Frameworks",
"@loader_path/Frameworks",
);
MACOSX_DEPLOYMENT_TARGET = 10.11;
MACOSX_DEPLOYMENT_TARGET = 12.0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please change it to 10.15 (through Carthage project settings)

LD_RUNPATH_SEARCH_PATHS = (
"$(inherited)",
"@executable_path/../Frameworks",
"@loader_path/Frameworks",
);
MACOSX_DEPLOYMENT_TARGET = 10.11;
MACOSX_DEPLOYMENT_TARGET = 12.0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please change it to 10.15 (through Carthage project settings)

@yaroslavyaroslav
Copy link
Collaborator

yaroslavyaroslav commented Apr 25, 2022

After fixing version issues i'm about to merge that massive PR in.

I've ran both tests suits locally and while remoteTests are all green 'localTests` have some issues, i thinks it's better than possible starting point of 3.0.0 release.

So i suggest to merge this boi and move further to make all tests green and then to start public API redesign.

@yaroslavyaroslav yaroslavyaroslav merged commit 3126c04 into web3swift-team:unstable Apr 27, 2022
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