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

[yaroslavyaroslav]

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

[janndriessen]

LGTM! 👍

[JeneaVranceanu]

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.

[yaroslavyaroslav]

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

[JeneaVranceanu]

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]

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]

@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]

@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]

@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]

@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]

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]

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]

@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]

@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]

@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]

@janndriessen please approve the PR and merge it within