feat: remote tests mocking #701

JeneaVranceanu · 2022-12-07T13:35:12Z

The reason for this PR: tests in #685 fail due to the Kovan testnet being deprecated and it requires a redeploy of all smart contracts we use for testing on actually running remote networks.

What is introduced in this PR?

Tests for ST20 and SecurtyToken completely rewritten to support mocking of network responses;
Web3.Eth implements protocol IEth. IEth protocol has default implementation for all network calls like sendTransaction, getBalance etc;
Web3.Eth is no longer references Web3 instance that created Web3.Eth instance (meaning there will be no memory leaks);
ContractProtocol extended with a new function that can guess and return ABI.Element.Function based on CodableTransaction.data without the need for developers to extract the method ID each time;
ST20 and SecurityToken tests no longer require network connection and thus no deployed smart contracts as well. We do not depend for these tests on a testnet like Goerli or Kovan, or any other;
ST20 and SecurityToken tests take 0.22 seconds to complete;
Added ABIEncoder tests for dynamic types string and byte. More tests will be added in upcoming PRs.

I've tried to use mock or make it easy to redeploy smart contracts on a new testnet.

Mocking libraries are not working in Swift as they do in other languages like e.g. Java. Swift has limitations on reflection and we cannot easily mock function calls and especially when these functions are in the extension block. I've tried using https://github.com/birdrides/mockingbird/ but in the end it failed in a sense that it will not allow to mock the parts we need due to how our code is written + Swift reflection limitations.

Another solution was to write "helper" functions that will deploy smart contracts on a selected chain. It has the following downsides:

cannot rely on a testnet being always available;
cannot rely on a network connection;
must setup for each individual chain an address and deposit some Ether to it;
must make sure there is enough balance (in a long run) on the addresses we use so that tests do not fail;
in a case of ST20 implementation we cannot test if the actual function call of a smart contract was created correctly (we can do so with the suggested changes in this PR).

The con of this update is that we have to implement mocks ourselves. It doesn't look difficult for now.

…ion called in a transaction

…web3

JeneaVranceanu · 2022-12-07T22:36:39Z

Tests/web3swiftTests/localTests/Mocks.swift

+    func callTransaction(_ transaction: CodableTransaction) async throws -> Data {
+        onCallTransaction?(transaction) ?? Data()
+    }
+}


If we need any other function to mock we just add it here.

JeneaVranceanu · 2022-12-07T22:43:15Z

Tests/web3swiftTests/localTests/ST20AndSecurityTokenTests.swift

+            switch function.name {
+            case "investorCount":
+                return ABIEncoder.encode(types: [.uint(bits: 256)], values: [expectedNumberOfInvestors] as [AnyObject])!
+//            case "investors":


Calling try await securityToken.investors() crashes.
The ABI and Swift implementations do not match. In ABI it expects uint256 argument as I recall but in Swift we try to call just investors function without arguments.

TODO: must fix the implementation.

Todo or Fixme? I mean do you planning to fix it within this PR or is it a task for some better times?

Not with this PR. I'll push a separate one as it's unrelated issue.

Removed commented code.
Fixing will be done in another PR.

yaroslavyaroslav · 2022-12-08T07:55:04Z

Sources/Core/Contract/ContractProtocol.swift

+    /// Given the transaction data searches for a match in ``ContractProtocol/methods``.
+    /// - Parameter data: encoded function call used in transaction data field. Must be at least 4 bytes long.
+    /// - Returns: function decoded from the ABI of this contract or `nil` if nothing was found.
+    func getFunctionCalled(_ data: Data) -> ABI.Element.Function?


My dream for 4.0.0 is to create a set of Ethereum only data types that will be accounts all that special cases.

Must be at least 4 bytes long.

Because while we're using some advantages of swift strong typed system in 3.0.0 we're still missing a lot of them, treating those Ethereum rules in a python way, by leaving notes to our future selves.

Nothing to say about rewriting ABI module to generics as well.

yaroslavyaroslav · 2022-12-08T07:58:35Z

Sources/Core/Contract/ContractProtocol.swift

+    public func getFunctionCalled(_ data: Data) -> ABI.Element.Function? {
+        guard data.count >= 4 else { return nil }
+        return methods[data[0..<4].toHexString().addHexPrefix()]?.first
+    }


Oh, I see your dirty hack here. This is somewhat that I'd like to be implemented as a protocol. But again this is dream not even about current century.

Not that dirty I'd say.
We have an array of bytes, we get the first 4 of them and then look them up in the dictionary.
It's pretty straightforward.

Sources/web3swift/EthereumAPICalls/Ethereum/IEth.swift

Sources/web3swift/Web3/Web3+Instance.swift

yaroslavyaroslav · 2022-12-08T08:28:09Z

Tests/web3swiftTests/localTests/ST20AndSecurityTokenTests.swift

+    override func setUp() async throws {
+        web3 = await Web3.InfuraGoerliWeb3(accessToken: Constants.infuraToken)
+        ethMock = Web3EthMock(provider: web3.provider)
+        web3.ethInstance = ethMock
+        st20token = ST20.init(web3: web3, provider: web3.provider, address: .contractDeploymentAddress())
+        securityToken = SecurityToken.init(web3: web3, provider: web3.provider, address: .contractDeploymentAddress())
+    }


Correct me if I'm wrong: Is the all mocking happening just here, or I missing something?

This is the setup for mocking. The thing that is identical for all tests.
But each individual test mocks callTransaction function and is able to intercept the incoming requests and decide what to return in each separate test.

yaroslavyaroslav · 2022-12-08T08:29:55Z

Tests/web3swiftTests/localTests/ST20AndSecurityTokenTests.swift

+            switch function.name {
+            case "investorCount":
+                return ABIEncoder.encode(types: [.uint(bits: 256)], values: [expectedNumberOfInvestors] as [AnyObject])!
+//            case "investors":


Todo or Fixme? I mean do you planning to fix it within this PR or is it a task for some better times?

yaroslavyaroslav · 2022-12-08T08:30:16Z

Tests/web3swiftTests/localTests/ST20AndSecurityTokenTests.swift

+
+        let investorsCount = try await securityToken.investorCount()
+        XCTAssertEqual(investorsCount, expectedNumberOfInvestors)
+//        XCTAssertEqual(investors, expectedInvestors)


Comments should be dropped though.

It's definitely not coming with this PR, just wanted to highlight that we have issues with either ABI or the implementation.
One of the conclusions that I have is that each and every ABI like Web3.Utils.st20ABI and the rest must be precisely the version of ABI that matches the original standard without any deviation.
Since we allow developers to reference these ABIs from our project it's our responsibility to:

validate the ABI (it must match the standard);

give the link to the place where we got this ABI from;

recommend developers to use their own ABIs if that's possible as they will have more control over that.

@yaroslavyaroslav @janndriessen Here is the fix for these commented out lines - #704

The branch with the fix is not based on the one in the current PR so I'll be able to update tests only when the fix is merged into develop.

janndriessen

Only one additional comment. Indeed the work load for mocking looks manageable. Thanks for introducing/working on this.

janndriessen · 2022-12-08T16:00:48Z

Sources/web3swift/Web3/Web3+Instance.swift

@@ -60,7 +57,7 @@ public class Web3 {
    // FIXME: Rewrite this to CodableTransaction
    public class Personal {
        var provider: Web3Provider
-        //        weak var web3: web3?
+        // FIXME: web3 must be weak


I guess this will be fixed in another PR?

Correct. Just made it more obvious that it needs fixing instead of a commented line of code that tells nothing.
I'll actually update the part after FIXME. Ideally we should remove web3 reference from here completely.

JeneaVranceanu · 2022-12-08T16:08:24Z

@yaroslavyaroslav @janndriessen Thanks for taking the time to look at this PR.
I'll push a few minor updates in 10 minutes based on your comments.

… instance!!"

janndriessen

🌟 ✅

JeneaVranceanu added 8 commits December 6, 2022 00:09

fix: return message from Web3Error.valueError

6a5ce15

fix: removed redundant spacing

b9ac465

feat: ContractProtocol - new function to search for ABI.Element.Funct…

60c9ca7

…ion called in a transaction

chore: added tests for dynamic types encoding

d08a2de

feat: mocking network responses

97ba8e9

chore: Web3.Eth + IEth refactoring

ff0778b

fix: added initializer for the Web3EthMock

29fff74

fix: changed type of web3.eth to IEth and removed it's dependency on …

e2b0f5d

…web3

JeneaVranceanu added the enhancement New feature or request label Dec 7, 2022

JeneaVranceanu requested review from janndriessen and yaroslavyaroslav December 7, 2022 13:35

JeneaVranceanu self-assigned this Dec 7, 2022

JeneaVranceanu commented Dec 7, 2022

View reviewed changes

yaroslavyaroslav reviewed Dec 8, 2022

View reviewed changes

janndriessen reviewed Dec 8, 2022

View reviewed changes

JeneaVranceanu added 2 commits December 8, 2022 18:11

chore: removed commented out case from SecurityToken test

bd2188a

chore: updated FIXME comments to "// FIXME: remove dependency on web3…

3707ca4

… instance!!"

janndriessen approved these changes Dec 8, 2022

View reviewed changes

yaroslavyaroslav approved these changes Dec 9, 2022

View reviewed changes

yaroslavyaroslav merged commit d036483 into web3swift-team:fix/contract-operations Dec 9, 2022

JeneaVranceanu deleted the feat/remote-tests-mocking branch December 9, 2022 12:33

This was referenced Dec 9, 2022

CI/CD Remote tests failing occasionally #649

Open

fix: contract operations #685

Merged

feat: remote tests mocking #701

feat: remote tests mocking #701

Uh oh!

Conversation

JeneaVranceanu commented Dec 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is introduced in this PR?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

janndriessen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JeneaVranceanu Dec 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JeneaVranceanu commented Dec 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

janndriessen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JeneaVranceanu commented Dec 7, 2022 •

edited

Loading

JeneaVranceanu Dec 8, 2022 •

edited

Loading

JeneaVranceanu commented Dec 8, 2022 •

edited

Loading