ContractProtocol: support for overloaded functions in ABI #587

JeneaVranceanu · 2022-07-07T08:51:58Z

⚠️ Contains breaking changes.

What does this PR introduce?

TL;DR

Support for overloaded functions in a smart contract. Tests added to prove it's working;
Fixed func deploy in ContractProtocol - wrong constructor was used to encode parameters;
Docs added for ContractProtocol;
ABI.Element.Function/Constructor now have their own dedicated encodeParameters and decodeInputData.
ABI.Element.Function also has it's own decodeReturnData.

Detailed description

In a solidity smart contract, we could have two functions with similar names but different input arguments.
It's basically function overloading.

The previous implementation of EthereumContract and declaration of protocol ContractProtocol were not supporting overloaded functions. If we had one two functions called myFunction the one that will be decoded last will be the only one that will be saved in methods dictionary and allMethds array as it will replace previously decoded one and mapped to name without input parameters.

New implementation makes sure:

All functions are decoded and none of them replaces the other as they are saved in a dictionary as - [String: [Function]];
Functions are also saved in this dictionary mapped not only to a name (e.g. myFunction) but also to a name with parameters' types (e.g. myFunction(uint256)), and to function signature - 4 bytes in HEX, lowercased with 0x prefix (e.g. 0xe164f2a0).

The list of breaking changes.

In ContractProtocol

allMethods type changed from [String] to [ABI.Element.Function];
allEvents type changed from [String] to [ABI.Element.Event];

In EthereumContract

it is no longer a struct but is a class.

Also, documentation was added to ContractProtocol.

How deploy function changed?

Another important change in ContractProtocol: deploy function expects constructor argument. This is the constructor of the smart contract you are attempting to deploy!

Before:

func deploy(bytecode: Data, parameters: [AnyObject], extraData: Data) -> EthereumTransaction?

After:

func deploy(bytecode: Data,
            constructor: ABI.Element.Constructor?,
            parameters: [AnyObject]?,
            extraData: Data?) -> EthereumTransaction?

This isn't a constructor that was decoded during the initialization of EthereumContract instance that was used previously. This is something that caught my eye as it was logically incorrect. Previously all deployment transactions were attempting to encode constructor parameters for the wrong constructor. Tests worked only because they were using constructors with no input arguments. There is one test where input arguments are used but it worked only because decoded ABI is having a constructor that has the same input arguments, in the same order as the bytecode of a smart contract that is being deployed.

Now both constructor and parameters must be passed in to the call of func deploy in order to call the constructor with parameters.

- Add language to code blocks - Replace Projects that are using lib to appropriate link to wiki. - Add myself to contributors list. - Some style enhancements.

…a for ABI.Element.Function and Constructor;

…in a JSON file to be attached to the project but there was none;

… input parameters

…tion.signature` (e.g. `getData(bytes32)`) - fatal error is thrown as ABI is invalid; Maybe it's worth replacing fatalError with just dropping all values but the first one from the saved array.

JeneaVranceanu · 2022-07-07T11:06:07Z

Sources/web3swift/Contract/EthereumContract.swift

-                continue
+            /// ABI cannot have two functions with exactly the same name and input arguments
+            if (methods[function.signature]?.count ?? 0) > 1 {
+                fatalError("Given ABI is invalid: contains two functions with possibly different return values but exactly the same name and input parameters!")


This is something I'm not sure about.
As an alternative solution, we could remove all duplicates and leave only the first or the last entry of some function in question. But then we won't be able to guarantee that attempting to encode/decode functions' input parameters and return values will work - that will depend on the ABI that was used during the initialization of EthereumContract.

Not my story, maybe @mloit have something to say?

I think it's generally bad to ignore issues and try to push forward based on some assumption or rule. I'm also not a fan of error reporting via halting the program either (Precondition, FatalError, etc). I would much prefer to throw an exception. I realize that's not necessarily possible here, as that might be a breaking change. So for the time being, I would stick with the halt as it is, I think that is far safer than continuing on and potentially creating an incorrect transaction that will cost someone.

I'll consider implementing this check within init and maybe declaring this variable, and other similar ones, as let instead.
This way we can return nil from initializers or throw an error with a detailed description.
All of these variables will be most likely accessed at least once, and it's unlikely there will be so huge smart contract that its ABI will take significant time to process. That's just another point in favor of changing all lazy vars into let (or at least not lazy).

Speaking about a let one, private variable required here with computed wrapper, because in reference types let will not guarantee constancy of value under it, it'll just guarantee the constancy of reference to it, so it could be changed occasionally by a user.

I think it doesn't apply in our case and here's why:

I've marked all variables as public let except transactionOptions and address. All collections are immutable now.

All types used in the following collections are structs: abi, methods, allMethods, events, allEvents. constructor is also struct. Thus, if anyone extracts it to a variable it will be a full copy.

Moreover - all types under ABI.Element have their internal variables declared as let which is good and must stay this way until web3 goes extinct.

So to recap - I do not see the actual issue here with marking variables as public let. If I didn't get the idea of your message let's discuss it.

JeneaVranceanu · 2022-07-07T11:09:39Z

Sources/web3swift/Web3/Web3+Contract.swift

@@ -16,7 +16,7 @@ extension web3 {

    /// Web3 instance bound contract instance.
    public class web3contract {
-        var contract: EthereumContract
+        private(set) public var contract: EthereumContract


This is another turning point.
I'm not sure of the reasons why var contract: EthereumContract wasn't initially public. It holds data that could be useful for developers and that is first of all decoded and filtered ABI.

I've looked at whole web3contract implementation why is it even variable? As i've seen in code there's none assigning to its happening except of init. Should we made it constant?

Good point. I'll take a look at this and other variables in web3contract and will propose changes making them constant if no new question will pop up during this additional review.
I wasn't sure if it should stay as var but also wanted to make it let. It's good that you suggested making it a constant 🙂

Done. Marked contract and web3 as public let.

yaroslavyaroslav · 2022-07-08T13:42:18Z

In favour of using new DocC tools that we're presented by Apple this summer please add some short code example how to use new contract deployment from user point of view. Please add it to the very first ABI declaration (or to that type which is the one with what user will iterate to make contract deployment. As example of such please take a look at unstable branch. (lines don't highlights within markdown markup for some reason https://github.com/skywinder/web3swift/blob/unstable/Sources/web3swift/API/APIMethod.swift#L16-L75)

This allows us to give a user some code example docs in convenient place (Xcode API reference palette).

JeneaVranceanu · 2022-07-08T13:45:04Z

In favour of using new DocC tools that we're presented by Apple this summer please add some short code example how to use new contract deployment from user point of view. Please add it to the very first ABI declaration (or to that type which is the one with what user will iterate to make contract deployment. As example of such please take a look at unstable branch.

This allows us to give a user some code example docs in convenient place (Xcode API reference palette).

Will do it later today.

Sources/web3swift/Web3/Web3+MutatingTransaction.swift

yaroslavyaroslav · 2022-07-08T13:28:24Z

Sources/web3swift/Contract/EthereumContract.swift

+    private(set) public lazy var methods: [String: [ABI.Element.Function]] = {
+        var methods = [String: [ABI.Element.Function]]()


This looks like computed variable, so there's could not be set modifier, coz we actually can't set it anyhow.

Probably not relevant anymore. Please, see the latest version of EthereumContract.

Sources/web3swift/Contract/EthereumContract.swift

yaroslavyaroslav · 2022-07-08T13:55:44Z

Sources/web3swift/Contract/EthereumContract.swift

    public var transactionOptions: TransactionOptions? = TransactionOptions.defaultOptions
    public var address: EthereumAddress? = nil

-    var _abi: [ABI.Element]
+    public let abi: [ABI.Element]


Just reminder that constant in reference type makes constant only the reference itself, not its values instead. So if it's required to deny user to modifying this filed we're should consider a private property with a public [computed] wrapper.

I'm not sure I get your point.
The EthereumContract was changed to class from struct.
All variables except address are marked as public let and thus cannot be modified.
All structs under ABI.Element are having their internal variables declared as public let (which is super good).

So there is no way for user of this class to modify something.

Sources/web3swift/Contract/EthereumContract.swift

yaroslavyaroslav · 2022-07-08T14:05:35Z

And also I'd be happy to hear some suggestion about name convention. Like this is definitely not a patch, but how bad it'll be to made 2.7.0 release?
I see that if this features were totally broken that's safe to just release it as 2.7.0 with release description from this PR. Like we couldn't broke something that's already have been broken.

@mloit @JeneaVranceanu

mloit · 2022-07-08T15:09:08Z

I think it's fine to make a 2.7.0 release.

…sions to a separate file from ContractProtocol.swift

…, getEvents and getConstructor functions added; chore: Added new error type for ABI filtering - ABIError.invalidFunctionOverloading that is thrown if array of [ABI.Element] contains two functions with same name and input parameters;

…o DefaultContractProtocol. The DefaultContractProtocol is inherited by EthereumContract;

…act; ContractProtocol 'init' now throws instead of returning 'nil';

…sed;

…'public let';

JeneaVranceanu · 2022-07-12T12:17:50Z

And also I'd be happy to hear some suggestion about name convention. Like this is definitely not a patch, but how bad it'll be to made 2.7.0 release? I see that if this features were totally broken that's safe to just release it as 2.7.0 with release description from this PR. Like we couldn't broke something that's already have been broken.

@mloit @JeneaVranceanu

@mloit @yaroslavyaroslav I'm not sure if 2.7.0 will be a good place for these changes as there are, not that many, but still breaking changes that can block people from moving on until they update their projects. I'd make it part of 3.0.0 (or some release candidate of 3.0.0).

I think since no one has reported any of the issues that were fixed here it'll be good to make it part of 3.0.0.

JeneaVranceanu · 2022-07-12T12:23:23Z

@yaroslavyaroslav
It's a bit funny but initially I didn't get your question about naming conventions correctly and thought you were referring to conventions of naming within Swift code.
As a result, I've quickly searched for naming conventions for Swift and found this - Swift Style Guide from Google. I think it's worth referring to as our guide for contributions. I've looked through main topics like File Names, String Literals, Source File Structure and General Formatting. Described rules seem reasonable to me.

But before we must decide something about SwiftLint that was added in these PRs (that look very similar):

…tion.signature` (e.g. `getData(bytes32)`) - fatal error is thrown as ABI is invalid; Maybe it's worth replacing fatalError with just dropping all values but the first one from the saved array.

…sions to a separate file from ContractProtocol.swift

…, getEvents and getConstructor functions added; chore: Added new error type for ABI filtering - ABIError.invalidFunctionOverloading that is thrown if array of [ABI.Element] contains two functions with same name and input parameters;

…o DefaultContractProtocol. The DefaultContractProtocol is inherited by EthereumContract;

…act; ContractProtocol 'init' now throws instead of returning 'nil';

…sed;

…'public let';

…omForEventPresence (typo in word Presence)

JeneaVranceanu · 2022-08-08T21:33:26Z

@yaroslavyaroslav Finally rebased to unstable!

JeneaVranceanu · 2022-08-09T07:49:10Z

Tests fail to run. Will check this.

… happen synchronously before test cases run. Fixed by using `func setUp() async throws` instead of `class func setUp()`.

JeneaVranceanu · 2022-08-09T18:56:02Z

Tests/web3swiftTests/localTests/LocalTestCase.swift

-            // check to see if we need to run the one-time setup
-            if isSetUp { return }
-            isSetUp = true
+    override func setUp() async throws {


@yaroslavyaroslav This is an important fix.
Because of the nature of async/await the blockchain instance that is deployed during testing doesn't get populated with new transactions before(!!) the actual test is executed - it all happens in parallel.
That led to CI failing because of gas estimation failure, which failed because of oracle failing to get the gas price in suggestGasFeeLegacy().

So now, before each test this function will run, but now it's synchronous.
So when the first test function will be executed by CI it will populate the blockchain instance with transactions, and the next test case will not add new transactions because of the condition on the line 18:

if block >= 25 { return }

yaroslavyaroslav · 2022-08-11T05:26:02Z

@JeneaVranceanu so I'm about to merge it into right away, are you ok with that?

JeneaVranceanu · 2022-08-11T05:43:45Z

Yes, let's do that.
Since all tests pass now it's safe to merge.

JeneaVranceanu · 2022-08-11T09:01:34Z

@yaroslavyaroslav I've some more updates I'd like to push but these will go as a continuation of this PR in order to avoid conflicts.
Waiting for this PR to be merged 🙂

Here is a preview - JeneaVranceanu#10

JeneaVranceanu · 2022-08-11T09:47:59Z

Thanks!

yaroslavyaroslav and others added 13 commits June 14, 2022 15:22

Update README.md

ce1c62f

- Add language to code blocks - Replace Projects that are using lib to appropriate link to wiki. - Add myself to contributors list. - Some style enhancements.

fix: ContractProtocol support for overloaded functions in ABI;

939904b

chore: updated docs of ContractProtocol

e43134c

chore: removed parenthesis

ded6121

feat: dedicated decodeInputData, encodeParameters and decodeReturnDat…

ebf43e7

…a for ABI.Element.Function and Constructor;

chore: updated project to support ContractProtocol changes

56e9228

chore: updated tests

8a34ab3

chore: removed commented test - it was expecting a specific keystore …

e07433d

…in a JSON file to be attached to the project but there was none;

chore: updated tests names - removed web3swift prefix

4638ebb

fix: wasn't appending function based on HEX and full method name with…

893e880

… input parameters

chore: if there are two or more functions with exactly the same `func…

75e2b2b

…tion.signature` (e.g. `getData(bytes32)`) - fatal error is thrown as ABI is invalid; Maybe it's worth replacing fatalError with just dropping all values but the first one from the saved array.

fix: allMethods must not return duplicates;

731723c

chore: added EthereumContractTest class to test new features;

1021b44

JeneaVranceanu changed the title ~~fix: ContractProtocol support for overloaded functions in ABI~~ ContractProtocol support for overloaded functions in ABI Jul 7, 2022

JeneaVranceanu commented Jul 7, 2022

View reviewed changes

yaroslavyaroslav reviewed Jul 8, 2022

View reviewed changes

JeneaVranceanu added 6 commits July 11, 2022 11:25

chore: moved EventFilter and all related classes, protocols and exten…

f969760

…sions to a separate file from ContractProtocol.swift

chore: moved default implementation of ContractProtocol functions int…

cc2c873

…o DefaultContractProtocol. The DefaultContractProtocol is inherited by EthereumContract;

chore: declared almost all variables as 'public let' in EthereumContr…

899bfc8

…act; ContractProtocol 'init' now throws instead of returning 'nil';

chore: removed transactionOptions from ContractProtocol - it wasn't u…

e38de3c

…sed;

chore: marked contract and web3 variables in class 'web3contract' as …

33a621c

…'public let';

JeneaVranceanu changed the title ~~ContractProtocol support for overloaded functions in ABI~~ ContractProtocol: support for overloaded functions in ABI Jul 12, 2022

JeneaVranceanu added 14 commits August 8, 2022 23:53

chore: if there are two or more functions with exactly the same `func…

49fe128

…tion.signature` (e.g. `getData(bytes32)`) - fatal error is thrown as ABI is invalid; Maybe it's worth replacing fatalError with just dropping all values but the first one from the saved array.

fix: allMethods must not return duplicates;

244c0b5

chore: added EthereumContractTest class to test new features;

b9a9341

chore: moved EventFilter and all related classes, protocols and exten…

dd50d83

…sions to a separate file from ContractProtocol.swift

chore: moved default implementation of ContractProtocol functions int…

a7d8021

…o DefaultContractProtocol. The DefaultContractProtocol is inherited by EthereumContract;

chore: declared almost all variables as 'public let' in EthereumContr…

cb7973a

…act; ContractProtocol 'init' now throws instead of returning 'nil';

chore: removed transactionOptions from ContractProtocol - it wasn't u…

e467aea

…sed;

chore: marked contract and web3 variables in class 'web3contract' as …

f5c5c6e

…'public let';

feat: added array of errors in ContractProtocol filtered from ABI;

07d64f8

fix: fixed typo in function name testBloomForEventPrecence -> testBlo…

d4df888

…omForEventPresence (typo in word Presence)

chore: documentation update with detailed guides for function deploy

dd11bbf

chore: missed entry in pbxproj

9943d2e

chore: merged with origin/fix/abi-overloading-methods-support

1bff58d

JeneaVranceanu changed the base branch from develop to unstable August 8, 2022 21:23

fix: removed incorrectly merged files

fb9f58f

JeneaVranceanu added 5 commits August 9, 2022 15:35

fix: removed EventFilter duplicate

48590e5

fix: tests fixed

b795104

fix: returned back incorrectly merged ERC20Tests

dee0b58

chore: changed XCTestCase to LocalTestCase on some test suites

d8f93e8

fix: LocalTestCase - pre-population of blockchain with 25 blocks must…

7c8b53c

… happen synchronously before test cases run. Fixed by using `func setUp() async throws` instead of `class func setUp()`.

JeneaVranceanu commented Aug 9, 2022

View reviewed changes

yaroslavyaroslav merged commit b8d54f6 into web3swift-team:unstable Aug 11, 2022

JeneaVranceanu deleted the fix/abi-overloading-methods-support branch August 30, 2022 08:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ContractProtocol: support for overloaded functions in ABI #587

ContractProtocol: support for overloaded functions in ABI #587

JeneaVranceanu commented Jul 7, 2022 •

edited

Loading

JeneaVranceanu Jul 7, 2022

yaroslavyaroslav Jul 8, 2022

mloit Jul 8, 2022

JeneaVranceanu Jul 9, 2022 •

edited

Loading

yaroslavyaroslav Jul 11, 2022 •

edited

Loading

JeneaVranceanu Jul 12, 2022

JeneaVranceanu Jul 7, 2022

yaroslavyaroslav Jul 8, 2022

JeneaVranceanu Jul 8, 2022

JeneaVranceanu Jul 12, 2022

yaroslavyaroslav commented Jul 8, 2022 •

edited

Loading

JeneaVranceanu commented Jul 8, 2022 •

edited

Loading

yaroslavyaroslav Jul 8, 2022

JeneaVranceanu Jul 12, 2022

yaroslavyaroslav Jul 8, 2022 •

edited

Loading

JeneaVranceanu Jul 12, 2022

yaroslavyaroslav commented Jul 8, 2022

mloit commented Jul 8, 2022

JeneaVranceanu commented Jul 12, 2022

JeneaVranceanu commented Jul 12, 2022

JeneaVranceanu commented Aug 8, 2022

JeneaVranceanu commented Aug 9, 2022

JeneaVranceanu Aug 9, 2022 •

edited

Loading

yaroslavyaroslav commented Aug 11, 2022

JeneaVranceanu commented Aug 11, 2022

JeneaVranceanu commented Aug 11, 2022 •

edited

Loading

JeneaVranceanu commented Aug 11, 2022

		private(set) public lazy var methods: [String: [ABI.Element.Function]] = {
		var methods = [String: [ABI.Element.Function]]()

ContractProtocol: support for overloaded functions in ABI #587

ContractProtocol: support for overloaded functions in ABI #587

Conversation

JeneaVranceanu commented Jul 7, 2022 • edited Loading

What does this PR introduce?

Detailed description

How deploy function changed?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeneaVranceanu Jul 9, 2022 • edited Loading

Choose a reason for hiding this comment

yaroslavyaroslav Jul 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaroslavyaroslav commented Jul 8, 2022 • edited Loading

JeneaVranceanu commented Jul 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaroslavyaroslav Jul 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaroslavyaroslav commented Jul 8, 2022

mloit commented Jul 8, 2022

JeneaVranceanu commented Jul 12, 2022

JeneaVranceanu commented Jul 12, 2022

JeneaVranceanu commented Aug 8, 2022

JeneaVranceanu commented Aug 9, 2022

JeneaVranceanu Aug 9, 2022 • edited Loading

Choose a reason for hiding this comment

yaroslavyaroslav commented Aug 11, 2022

JeneaVranceanu commented Aug 11, 2022

JeneaVranceanu commented Aug 11, 2022 • edited Loading

JeneaVranceanu commented Aug 11, 2022

JeneaVranceanu commented Jul 7, 2022 •

edited

Loading

JeneaVranceanu Jul 9, 2022 •

edited

Loading

yaroslavyaroslav Jul 11, 2022 •

edited

Loading

yaroslavyaroslav commented Jul 8, 2022 •

edited

Loading

JeneaVranceanu commented Jul 8, 2022 •

edited

Loading

yaroslavyaroslav Jul 8, 2022 •

edited

Loading

JeneaVranceanu Aug 9, 2022 •

edited

Loading

JeneaVranceanu commented Aug 11, 2022 •

edited

Loading