fix contract bugs #682

pharms-eth · 2022-11-21T15:39:37Z

No description provided.

Sources/web3swift/Operations/WriteOperation.swift

JeneaVranceanu · 2022-11-21T21:24:34Z

Sources/web3swift/Operations/WriteOperation.swift

+    ///   - sendRaw: If set to `true` transaction will be signed and sent using `eth_sendRawTransaction`.
+    ///   Otherwise, no signing attempts will take place and the `eth_sendTransaction` RPC will be used instead.
+    ///   Default value is `false`.
+    public func writeToChain(password: String, policies: Policies = .auto, sendRaw: Bool = false) async throws -> TransactionSendingResult {


I'm still not sure why would we set default value for sendRaw to false.
One issue I see clearly now with this implementation - everyone who uses the library will suddenly start sending unsigned transactions to the blockchain and that will basically break everything for those developers.
So I think in the current state this PR won't pass.

@pharms-eth Could you please either paste a link here or maybe describe once again why did you decide to set the value to false?

Probably found it - https://github.com/web3swift-team/web3swift/pull/670/files#r1027680672

So this was the issue?

yes setting it true by default makes the tests fail

yea this can't be the reasoning. pharms would have to investigate why the tests fail then.

I see it like you @JeneaVranceanu we can't just change this now.

ok I will investigate the failed tests

janndriessen · 2022-11-21T21:27:34Z

Sources/Core/EthereumABI/ABIElements.swift

@@ -325,7 +325,7 @@ extension ABI.Element.Function {
            // set a flag to detect the request succeeded
        }

-        if returnArray.isEmpty {
+        if returnArray.isEmpty && !outputs.isEmpty {


I know I'm annoying with this but can we please make a test here. 😄

I will work on this

Just wonder, does it resolved yet, @pharms-eth?

JeneaVranceanu · 2022-11-22T08:36:25Z

The following test fails:

Test Suite 'AdvancedABIv2Tests' started at 2022-11-22 02:20:08.216
Test Case '-[localTests.AdvancedABIv2Tests testAdvancedABIv2dynamicArrayOfStrings]' started.
localTests/LocalTestCase.swift:33: Fatal error: 'try!' expression unexpectedly raised an error: Core.Web3Error.inputError(desc: "Failed to locally sign a transaction. Web3 provider doesn\'t have keystore attached.")

@pharms-eth It appears to be that deployment of a contract in this test requires us to set sendRaw to false.
So just do the following:

JeneaVranceanu · 2022-11-22T08:57:16Z

@pharms-eth Please run all tests locally. You will need ganache installed, it should be easy to do - https://trufflesuite.com/ganache/

After that run ganache command in the terminal and you are ready to run all tests. See if other tests fail too. I suppose almost all of the tests in the AdvancedABIv2Tests will fail due to that change.

pharms-eth · 2022-11-22T16:10:47Z

@pharms-eth Please run all tests locally. You will need ganache installed, it should be easy to do - https://trufflesuite.com/ganache/

After that run ganache command in the terminal and you are ready to run all tests. See if other tests fail too. I suppose almost all of the tests in the AdvancedABIv2Tests will fail due to that change.

I do not use truffle products, the quality of their work is not good, So i will not want to install locally and pollute my machine. however the errors now are vague and so may require it

JeneaVranceanu · 2022-11-23T16:12:49Z

@pharms-eth There are at least 10 other tests that fail.

Please open this job run and search by Failed to locally sign a transaction. You will see which tests have failed. You will need to do the same sendRaw: false fix for them.

At the same time, I'll check why do we need sendRaw: false there in these failed tests in the first place.

janndriessen · 2022-11-23T18:41:17Z

@pharms-eth There are at least 10 other tests that fail.

Please open this job run and search by Failed to locally sign a transaction. You will see which tests have failed. You will need to do the same sendRaw: false fix for them.

At the same time, I'll check why do we need sendRaw: false there in these failed tests in the first place.

This what I was thinking is sendRaw: false even the correct fix?

yaroslavyaroslav · 2022-11-24T10:10:39Z

I just wonder, why they fails? Is it on ganache's side or is it us? Like why it it matter on the test side? And I think it's worth to add a test to covering this new writeToChain(raw:) behaviour. But looking at all those fails (if it's not us) is it even possible to do so locally?

yaroslavyaroslav · 2022-11-24T12:09:48Z

Just to highlight it once more: this one will be the last breaking change to be accepted within 3.*.*

pharms-eth · 2022-11-24T17:31:18Z

@yaroslavyaroslav @janndriessen a lot of the failures are because under this change we need the tests setup with the keystore manager. but because they are not and the default is set to use that path, it fails

janndriessen · 2022-11-25T07:45:39Z

To be able to use signed transactions, shouldn't it be something like this in every test?

        let privateKey = "" // add from ganache
        let privateKeyData = Data.fromHex(privateKey)!
        let keystore = try! EthereumKeystoreV3(privateKey: privateKeyData, password: password)
        let keystoreManager = KeystoreManager([keystore!])
        web3.addKeystoreManager(keystoreManager)
        let ownersAddress = keystoreManager.addresses![0]

(Code not tested.)

What do you think? @pharms-eth @JeneaVranceanu @yaroslavyaroslav

JeneaVranceanu · 2022-11-25T12:45:34Z

To be able to use signed transactions, shouldn't it be something like this in every test?
        let privateKey = "" // add from ganache
        let privateKeyData = Data.fromHex(privateKey)!
        let keystore = try! EthereumKeystoreV3(privateKey: privateKeyData, password: password)
        let keystoreManager = KeystoreManager([keystore!])
        web3.addKeystoreManager(keystoreManager)
        let ownersAddress = keystoreManager.addresses![0]
(Code not tested.)

What do you think? @pharms-eth @JeneaVranceanu @yaroslavyaroslav

I've tried something like that in one of the tests and it failed. Not sure if it was exactly this configuration.
I'll look now into this issue.

JeneaVranceanu · 2022-11-25T14:15:24Z

@yaroslavyaroslav @janndriessen @pharms-eth
TL;DR: sendRaw: false as a fix for failing tests was the correct solution.

Reasoning: we are using Ganache for testing (and I think other platforms that spin up blockchains/nodes locally for testing do the same) and when it launches a network it generates a list of private keys, thus a list of addresses, that we can use. To access this list we can call eth_getAccounts which is happening in these tests.
We take the first account in the list and send an unsigned transaction using eth_sendTransaction with from address set to the first address from the list we previously received.
Ganache picks up that transactions, sees that it was called using eth_sendTransaction and looks up the address specified in the from field. That address is used to identify which private key stored by Ganache must be used to sign that transaction.
The transaction gets signed and added to the next available block and successfully executes.

So this solution with attaching key store manager would work as well but then we must use sendRaw: true which is set by default. We also must not use eth_getAccounts in that case and derive the address from the private key in the key store we own. It would be somewhat more inconvenient since it's not needed in each individual test case.

Conclusion: additionally, besides fixing tests by using sendRaw: false, we must add more tests that explicitly use sendRaw: true and tests that omit this argument.

yaroslavyaroslav

I'm good with this PR except that one issue with condition enhancement without covering it by the test.

I suppose it should be either covered right here right now or issue about that has to be created.

janndriessen · 2022-11-26T14:01:50Z

I'm good with this PR except that one issue with condition enhancement without covering it by the test.

I suppose it should be either covered right here right now or issue about that has to be created.

Personally, I think it's too important part of the library to not looking into it now as it could be breaking changes too. I'm not familiar with that area but could look into it next week. Though @JeneaVranceanu told me via DM that he's looking into this as well.

JeneaVranceanu · 2022-11-26T14:03:50Z

Personally, I think it's too important part of the library to not looking into it now as it could be breaking changes too. I'm not familiar with that area but could look into it next week. Though @JeneaVranceanu told me via DM that he's looking into this as well.

I'm writing a test for it right now. Will push it as a separate branch.

JeneaVranceanu · 2022-11-26T14:07:22Z

@janndriessen I've changed the base branch of this PR so I can push the tests in a PR based on this branch without the need for re-review.
This one will be merged now into fix/contract-operations which is based on the current development branch.

fix bugs

b374a34

pharms-eth requested review from janndriessen, yaroslavyaroslav and JeneaVranceanu November 21, 2022 15:39

fix: documentation update

e4629c3

JeneaVranceanu reviewed Nov 21, 2022

View reviewed changes

Sources/web3swift/Operations/WriteOperation.swift Outdated Show resolved Hide resolved

fix: documentation update for func writeToChain

93372dd

JeneaVranceanu requested changes Nov 21, 2022

View reviewed changes

janndriessen reviewed Nov 21, 2022

View reviewed changes

update per feedback

8040c7a

pharms-eth added 2 commits November 22, 2022 07:35

attempt test fix

1fc9285

working on tests

d143bf1

pharms-eth added 3 commits November 22, 2022 09:08

restore true

42eed72

amend tests

28147e8

adjust helpers

e42e0f3

pharms-eth added 2 commits November 24, 2022 10:30

adjust test for deploy changes

5bd13c4

fix another test for deploy changes

94d9963

pharms-eth requested review from janndriessen and JeneaVranceanu November 24, 2022 21:40

chore: default value of sendRaw for writeToChain is true

cf534c0

JeneaVranceanu approved these changes Nov 25, 2022

View reviewed changes

yaroslavyaroslav reviewed Nov 26, 2022

View reviewed changes

JeneaVranceanu changed the base branch from develop to fix/contract-operations November 26, 2022 14:05

JeneaVranceanu merged commit 6247628 into web3swift-team:fix/contract-operations Nov 26, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix contract bugs #682

fix contract bugs #682

pharms-eth commented Nov 21, 2022

JeneaVranceanu Nov 21, 2022 •

edited

Loading

JeneaVranceanu Nov 21, 2022

janndriessen Nov 21, 2022

pharms-eth Nov 22, 2022

janndriessen Nov 21, 2022

JeneaVranceanu Nov 21, 2022

pharms-eth Nov 24, 2022

yaroslavyaroslav Nov 26, 2022

JeneaVranceanu commented Nov 22, 2022

JeneaVranceanu commented Nov 22, 2022

pharms-eth commented Nov 22, 2022 •

edited

Loading

JeneaVranceanu commented Nov 23, 2022

janndriessen commented Nov 23, 2022

yaroslavyaroslav commented Nov 24, 2022

yaroslavyaroslav commented Nov 24, 2022

pharms-eth commented Nov 24, 2022

janndriessen commented Nov 25, 2022

JeneaVranceanu commented Nov 25, 2022

JeneaVranceanu commented Nov 25, 2022 •

edited

Loading

yaroslavyaroslav left a comment

janndriessen commented Nov 26, 2022

JeneaVranceanu commented Nov 26, 2022 •

edited

Loading

JeneaVranceanu commented Nov 26, 2022

fix contract bugs #682

fix contract bugs #682

Conversation

pharms-eth commented Nov 21, 2022

JeneaVranceanu Nov 21, 2022 • edited Loading

Choose a reason for hiding this comment

JeneaVranceanu Nov 21, 2022

Choose a reason for hiding this comment

janndriessen Nov 21, 2022

Choose a reason for hiding this comment

pharms-eth Nov 22, 2022

Choose a reason for hiding this comment

janndriessen Nov 21, 2022

Choose a reason for hiding this comment

JeneaVranceanu Nov 21, 2022

Choose a reason for hiding this comment

pharms-eth Nov 24, 2022

Choose a reason for hiding this comment

yaroslavyaroslav Nov 26, 2022

Choose a reason for hiding this comment

JeneaVranceanu commented Nov 22, 2022

JeneaVranceanu commented Nov 22, 2022

pharms-eth commented Nov 22, 2022 • edited Loading

JeneaVranceanu commented Nov 23, 2022

janndriessen commented Nov 23, 2022

yaroslavyaroslav commented Nov 24, 2022

yaroslavyaroslav commented Nov 24, 2022

pharms-eth commented Nov 24, 2022

janndriessen commented Nov 25, 2022

JeneaVranceanu commented Nov 25, 2022

JeneaVranceanu commented Nov 25, 2022 • edited Loading

yaroslavyaroslav left a comment

Choose a reason for hiding this comment

janndriessen commented Nov 26, 2022

JeneaVranceanu commented Nov 26, 2022 • edited Loading

JeneaVranceanu commented Nov 26, 2022

JeneaVranceanu Nov 21, 2022 •

edited

Loading

pharms-eth commented Nov 22, 2022 •

edited

Loading

JeneaVranceanu commented Nov 25, 2022 •

edited

Loading

JeneaVranceanu commented Nov 26, 2022 •

edited

Loading