`ERC20.transaction` doesn't assigns to `WriteOperation.transaction` property fix #713

yaroslavyaroslav · 2022-12-15T07:55:32Z

This one fixing the ERC20 implementation bug, where ERC20.transaction never assigns to Contract.transaction for any given method that writes on chain.

Obviously it's a bug spreading all over the Token implementation. So this fix should be applied to all ERC*.swift methods that returns WriteOperation.

This is the special case of #712.

Closes #711

…n` never assigns to `Contract.transaction` for any given method that writes on chain. Obviously it's a bug spreading all over the `Token` implementation. So this fix should be applied to all `ERC*.swift` methods that returns `WriteOperation`. This is the special case of web3swift-team#712. Closes web3swift-team#711

janndriessen

LGTM! 👍

JeneaVranceanu · 2022-12-19T18:31:34Z

Sources/web3swift/Tokens/ERC20/Web3+ERC20.swift

@@ -80,12 +79,12 @@ public class ERC20: IERC20, ERC20BaseProperties {
        guard let value = Utilities.parseToBigUInt(amount, decimals: intDecimals) else {
            throw Web3Error.inputError(desc: "Can not parse inputted amount")
        }
+        contract.transaction = transaction


Why don't we do the same in all of the functions left?
For example, check public func setAllowance. It looks almost identical to public func transfer in terms of the logic of each step inside the function: set transaction from, to and callOnBlock ; call read-only "decimals" function; create WriteOperation.

This one is actually a bug that I've left. It's fixed just yet.

JeneaVranceanu · 2022-12-19T18:34:51Z

I'd completely remove transaction property from Contract. It's a perfect place for side-effects and other uncatchable errors.
I'll actually strongly advocate for removing this property.
If you could give me some pros about having it I'd gladly read that.

@yaroslavyaroslav @janndriessen

janndriessen · 2022-12-20T07:31:21Z

I'd completely remove transaction property from Contract. It's a perfect place for side-effects and other uncatchable errors. I'll actually strongly advocate for removing this property. If you could give me some pros about having it I'd gladly read that.

@yaroslavyaroslav @janndriessen

For the long term I agree (and I would actually completely rewrite the ERC20 class) but as I understood Yaroslav this was only supposed to be a quick fix. Also about your other comments, he articulated that this will have to be done for all classes in #712.

JeneaVranceanu · 2022-12-20T08:49:52Z

@janndriessen
As a quick fix, it's completely acceptable. But I'll open a separate issue with links to this PR and the comment from above about removing that transaction object from contract class.
It will be open for a discussion.

JeneaVranceanu · 2022-12-20T08:51:21Z

@yaroslavyaroslav But I still think we should update the rest of the functions across this and other ERCs implementations we have. Because it's basically the same bug that is repeating from one function to the other.

JeneaVranceanu · 2022-12-21T12:52:59Z

@yaroslavyaroslav I think this PR is sort of ready to be merged but a have a few concerns that were reflected in my comment above.
To rephrase my comment #713 (comment): I think we should either update all other functions in this PR or in another PR that is forked out of this branch and then we are good to merge.

janndriessen · 2022-12-21T16:01:50Z

@yaroslavyaroslav I think this PR is sort of ready to be merged but a have a few concerns that were reflected in my comment above. To rephrase my comment #713 (comment): I think we should either update all other functions in this PR or in another PR that is forked out of this branch and then we are good to merge.

I'll look into adding a PR on top for that.

yaroslavyaroslav · 2022-12-22T08:25:29Z

I'd completely remove transaction property from Contract. It's a perfect place for side-effects and other uncatchable errors.

The designed pipeline of contract iteration flow was:
ERC+*** -> Contract -> [Read|Write]Operation.

The intention that I've got in minds while refactoring this bit was:

The main intent of this bit of refactoring was to provide unified pipeline to both create, setup and send transaction when it comes to the contract interaction. Therefore there was 3 major classes has been kept: [Read|Write]Operation, Contract, ERC+***.

The ERC+*** classes are treated by me as just a convenient wrappers around the bare bone EthereumContract one. So the Contract type which is inherits of it treated by me the same: as a some convenient yet general wrapper around the EthereumContract.

In the given pipeline *Operation awaits that there would be some *Contract instance provided that is incapsulates all Contract related setup and that should be encoded and sent to a chain to made a success contract iteration.

So to sum up: tx property of a Contract type is provided there to make things a bit more consistent. Actually I had an idea to drop all ERC+*** tx and contract implementation detail and made it belongs to Contract one.

yaroslavyaroslav · 2022-12-22T08:28:41Z

As a quick fix, it's completely acceptable. But I'll open a separate issue with links to this PR and the comment from above about removing that transaction object from contract class.

A contrary question here is, how the user would be manage an arbitrary (Non ERC+* listed contract) without the tx property? Like do you assume that they will be setup it in WriteOperation.transaction directly?

yaroslavyaroslav · 2023-01-10T16:02:17Z

@JeneaVranceanu I guess we've paused this topic for a holiday, but it's still required to be done, to present appropriate bug-fix.

janndriessen · 2023-01-10T16:31:21Z

@JeneaVranceanu I guess we've paused this topic for a holiday, but it's still required to be done, to present appropriate bug-fix.

Do you see anything else to be done besides the clean up in #715?

JeneaVranceanu · 2023-01-10T18:33:28Z

@JeneaVranceanu I guess we've paused this topic for a holiday, but it's still required to be done, to present appropriate bug-fix.

@yaroslavyaroslav @janndriessen It looks good now. I've left a review in #715 but this particular PR is ready to be merged.

yaroslavyaroslav · 2023-01-13T15:43:01Z

@janndriessen please approve the PR and merge it within

yaroslavyaroslav changed the title ~~ERC20.transaction doesn't assigns to WriteOperation.transaction property~~ ERC20.transaction doesn't assigns to WriteOperation.transaction property fix Dec 15, 2022

yaroslavyaroslav mentioned this pull request Dec 15, 2022

WriteOperation.transaction ignores ERC*.transaction settings #712

Closed

yaroslavyaroslav requested review from janndriessen and JeneaVranceanu December 15, 2022 08:08

janndriessen previously approved these changes Dec 18, 2022

View reviewed changes

JeneaVranceanu reviewed Dec 19, 2022

View reviewed changes

Fixing review issues.

aa4d563

yaroslavyaroslav dismissed janndriessen’s stale review via aa4d563 December 22, 2022 07:58

JeneaVranceanu approved these changes Jan 10, 2023

View reviewed changes

JeneaVranceanu merged commit aa4d563 into web3swift-team:develop Jan 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`ERC20.transaction` doesn't assigns to `WriteOperation.transaction` property fix #713

`ERC20.transaction` doesn't assigns to `WriteOperation.transaction` property fix #713

yaroslavyaroslav commented Dec 15, 2022

janndriessen left a comment

JeneaVranceanu Dec 19, 2022

yaroslavyaroslav Dec 22, 2022 •

edited

Loading

JeneaVranceanu commented Dec 19, 2022

janndriessen commented Dec 20, 2022

JeneaVranceanu commented Dec 20, 2022

JeneaVranceanu commented Dec 20, 2022

JeneaVranceanu commented Dec 21, 2022

janndriessen commented Dec 21, 2022

yaroslavyaroslav commented Dec 22, 2022

yaroslavyaroslav commented Dec 22, 2022 •

edited

Loading

yaroslavyaroslav commented Jan 10, 2023

janndriessen commented Jan 10, 2023

JeneaVranceanu commented Jan 10, 2023 •

edited

Loading

yaroslavyaroslav commented Jan 13, 2023

ERC20.transaction doesn't assigns to WriteOperation.transaction property fix #713

ERC20.transaction doesn't assigns to WriteOperation.transaction property fix #713

Conversation

yaroslavyaroslav commented Dec 15, 2022

janndriessen left a comment

Choose a reason for hiding this comment

JeneaVranceanu Dec 19, 2022

Choose a reason for hiding this comment

yaroslavyaroslav Dec 22, 2022 • edited Loading

Choose a reason for hiding this comment

JeneaVranceanu commented Dec 19, 2022

janndriessen commented Dec 20, 2022

JeneaVranceanu commented Dec 20, 2022

JeneaVranceanu commented Dec 20, 2022

JeneaVranceanu commented Dec 21, 2022

janndriessen commented Dec 21, 2022

yaroslavyaroslav commented Dec 22, 2022

yaroslavyaroslav commented Dec 22, 2022 • edited Loading

yaroslavyaroslav commented Jan 10, 2023

janndriessen commented Jan 10, 2023

JeneaVranceanu commented Jan 10, 2023 • edited Loading

yaroslavyaroslav commented Jan 13, 2023

`ERC20.transaction` doesn't assigns to `WriteOperation.transaction` property fix #713

`ERC20.transaction` doesn't assigns to `WriteOperation.transaction` property fix #713

yaroslavyaroslav Dec 22, 2022 •

edited

Loading

yaroslavyaroslav commented Dec 22, 2022 •

edited

Loading

JeneaVranceanu commented Jan 10, 2023 •

edited

Loading