Feature/automatic gas 1559

[mloit]

This PR enables handling of .automatic for the new gas parameters on an EIP-1559 transaction.
The existing handler was refactored to use the new Oracle for gas pricing
This PR also fixes #546 by preserving transaction type in the transaction rewrite

[mloit]

Strange, not sure why the tests are failing here, they pass when I run it on my system

[yaroslavyaroslav]

I can't confirm that. Coz when i ran first of failed tests in ci pipeline testERC20tokenBalancePromise it crashed in runtime.

The reason of crashing is clear: it runs GasOracle prediction on empty ganache blockchain, whenever Oracle object with default settings expecting at least 20 block back from now. So it's just get negative UInt value error here, but it's technical reason, which triggers from here

I suggest you to add some checks that there's at least that much blocks in blockchain when you've creating Oracle instance and set it's blockCount property to that number.

Meanwhile i'll add check in Oracle.suggestGasFeeLegacy property whether latestBlockNumber > blockCount and return empty array in it's not.

I'm not sure how tests will be going after that, but at least there'll be no crash on it.

[mloit]

Oh... yeah that could be. I tend to have my Ganache always running in the background, so it would have a fair amount of history.

Instead of returning nil if the chain block number is too low, maybe return based on all the blocks available. Reality is this is an edge case that only happens in testing. ie, if there are only 5 blocks on chain, return the result based on those 5 blocks, even if Oracle was initialized to use 20

We don't easily have control over the number of blocks that may be present on the Ganache chain. So we can't really count on previous tests having been run to generate a certain amount of traffic. (tests are run in parallel,and possibly in random order)

[yaroslavyaroslav]

Ganache starts locally from scratch on every tests run. So it's always empty. I still wonder why tests have been green from the start in this setup, but it is what it is.

So i've checked my hypothesis and when i've update method check it stop crushing in that case, but test becomes red. All of them i guess and there's pretty much there.

So now we're having some wired situation when seems that we have some not quite reliable localTests. Actualy i don't know what to do yet.

Actually this thing happening since method that we testing in localTest test suit calls method that we test in remoteTest test suit. So latter are not designed to be tested on ganache, when former do.

Also i'm thinking that we shouldn't update any of our code to cover this situation (when our lib runs on empty blockchain) since it's almost unbelievable in real life usecases.

So i guess we should do something with tests, but i wonder what?

1. We could make them remote, but some of them are about contract deployment, e.g. paid.
2. We could add some mocks to ganache setup, but i'm afraid that this is a large amount of work, since we'll have to update a lot of local tests (i guess) to make them fit to exact ganache state (e.g. account addresses, blocks numbers and so on).

Any ideas [what to do with write in blockchain tests]?

[mloit]

Ganache starts locally from scratch on every tests run. So it's always empty. I still wonder why tests have been green from the start in this setup, but it is what it is.

So i've checked my hypothesis and when i've update method check it stop crushing in that case, but test becomes red. All of them i guess and there's pretty much there.

So now we've have some wired situation when seems that we have some not quite reliable localTests. Actualy i don't know what to do yet.

Also i'm thinking that we shouldn't update any of our code to cover this situation (when our lib runs on empty blockchain) since it's almost unbelievable in real life usecases.

So i guess we should do something with tests, but i wonder what?

1. We could make them remote, but some of them are about contract deployment, e.g. paid.

2. We could add some mocks to ganache setup, but i'm afraid that this is a large amount of work, since we'll have to update a lot of local tests (i guess) to make them fit to exact ganache state (e.g. account addresses, blocks numbers and so on).

Any ideas?

It was okay before, because it wasn't using Oracle, and thus did not need historical blocks to return a result. It simply used the getGasPricePromise function.

I agree we should not be modifying the main code to handle this odd case which should happen in testing only.

The solution here is to add a class (not instance) setUp() function to check the block count, and add transactions if the number is too low.

How to do one-time setup for your tests

[yaroslavyaroslav]

The solution here is to add a class (not instance) setUp() function to check the block count, and add transactions if the number is too low.

We're facing not swift problem, but blockchain problem here.

Like it's even possible to save ganache blockchain state through the ci/cd runs, but the problem is that we're needed to generate enough transactions for 21 blocks at least and consider different situations. And this is the place where i've gave up.

If you have an tool to generate such number of diverse transactions for ganache i'll could setup ganache for ci/cd pipeline easily

[mloit]

We're facing not swift problem, but blockchain problem here.

Yes, but it's a problem we can solve in Swift. By creating a setUp function we can initialize Ganache with additional transactions so that we are in a state that is good for our tests. This is exactly what setUp is for. Now sometimes it's good to do that for each test, but in our case we only want to do it once, which is why we do the class, and not instance, version of the set-up.

We can easily generate any number if transactions in a loop to initialize Ganache, and it is pretty easy to bypass the code that is causing us problems. Either by writing raw transactions, or by creating transactions with manual settings for the gas parameters (we only fail when we have to resolve .automatic).

Having the setUp code is better than trying to save ganache in some state, as it ensures that anyone trying to run the tests will have a good state, and not have to take special steps to set things up.

[mloit]

Ok I've added a pre-loader setUp function, for now. It's not the prettiest thing, but seems to work. It will only run up to 25 blocks. If there are 25 or more blocks it will exit without doing anything. The down side is the code needs to be added to each test class that depends on the blockchain state. Though I put the working part of the code in an external function, so the replicated code is simple and small.

This thread discusses some better options for running one-time setup, which we should explore for a future PR / improvement
How to run one-time setup code before executing any XCTest

[mloit]

Refactored the tests a bit, so they automatically inherit the setUp, slightly less ugly now.
Also did a little light lint clean up on the worst of the files.

[yaroslavyaroslav]

Having the setUp code is better than trying to save ganache in some state, as it ensures that anyone trying to run the tests will have a good state, and not have to take special steps to set things up.

I'm disagree with that, i think that

1. Persistent state of test mocks are important
2. Moving blockchain level problem to swift level is not best practice.

So i suggest to use your code to one-time generate some persistent state and save it within test folder and make ganache to initiate itself according to that state in ci/cd config.

This made possible to user to look ci/cd path when they wants to run tests locally and reproduce it themselves, since it would be still single line of code with path where to read, not some massive yml config.

[mloit]

Having the setUp code is better than trying to save ganache in some state, as it ensures that anyone trying to run the tests will have a good state, and not have to take special steps to set things up.

I'm disagree with that, i think that

1. Persistent state of test mocks are important

2. Moving blockchain level problem to swift level is not best practice.

So i suggest to use your code to one-time generate some persistent state and save it within test folder and make ganache to initiate itself according to that state in ci/cd config.

This made possible to user to look ci/cd path when they wants to run tests locally and reproduce it themselves, since it would be still single line of code with path where to read, not some massive yml config.

And how does this work when someone is developing within XCode? using Command+U to build and run tests will no longer work, as it does not start Ganache. The user is expected to start Ganache (likely via the GUI), and now they would need to somehow add the necessary number of blocks before the tests can be successfully run. Now for the XCode project you may be able to get a script to run to launch Ganache with a particular dataset before tests are run, but you would not be able to do this with SPM.

Having said that, I will reverse out my changes to the tests here, this will cause it to fail again. I will submit a new PR with a new test group that can be used to prime Ganache with the blocks. The new PR will need to be applied, and your changes to the ci/cd will need to be made before this one will pass tests.

[mloit]

Okay, I've reverted here, and added the generator in PR #549. Now all we need is for ci/cd to be updated so this one can pass the tests again.

1. Persistent state of test mocks are important

I don't think it needs to be persistent, it just needs to be consistent. And what I had before would have satisfied that, as on each run of the tests, Ganache would start at 0, and then the code would add exactly the same 25 blocks to the chain before proceeding with testing, creating the exact same environment for each test session.

2. Moving blockchain level problem to swift level is not best practice.

This is neither a Swift problem, or a blockchain problem... this is a testing problem. There is nothing wrong with having some code to create a suitable environment for your testing to proceed. In fact it's pretty uncommon not to have some sort of code to initialize/create that environment. Sometimes it's per test, sometimes it's per session.

[yaroslavyaroslav]

And how does this work when someone is developing within XCode? using Command+U to build and run tests will no longer work, as it does not start Ganache.

This actually don't works here either. Like every user have to run ganache manually (my thought is by cli) to make any local tests pass.

The user is expected to start Ganache (likely via the GUI), and now they would need to somehow add the necessary number of blocks before the tests can be successfully run.

My assumptions is follow:

• user (developer) will install ganache via npm
• user will run one-liner ganache --database.dbPath ./Tests/db.db instead of ganache

This is (passing argument to ganache) is not that simple as just run ganache without any arguments, but it seems that both this case will made new contributor struggle.

Now for the XCode project you may be able to get a script to run to launch Ganache with a particular dataset before tests are run, but you would not be able to do this with SPM.

I'm thinking that we should not move any additional setup of CI/CD pipeline into Xcode project rather SPM (seems it'll be possible starting from Swift 5.7). I'm seeing this picture:

1. separate most atomic tools that solves exact tasks, like build, setup environment, test.
2. ci/cd just connect all of those into one pipeline.
3. user (contributor) uses all tools from point 1 manually, without any additional automated pipelines.

[mloit]

And how does this work when someone is developing within XCode? using Command+U to build and run tests will no longer work, as it does not start Ganache.

This actually don't works here either. Like every user have to run ganache manually (my thought is by cli) to make any local tests pass.

Can you clarify what you mean by "don't work". If I start Ganache, from the Ganache UI (not from command line) do a "Ethereum Quickstart" And then I can run the tests Command+U, and they do work (Works in XCode for both .xcproj and SPM). Now for this PR, I do need the additional history, because of Oracle - as discussed earlier. My set-up code I had provided exactly that.

The user is expected to start Ganache (likely via the GUI), and now they would need to somehow add the necessary number of blocks before the tests can be successfully run.

My assumptions is follow:

* user (developer) will install ganache via npm

* user will run one-liner `ganache   --database.dbPath ./Tests/db.db` instead of `ganache`

Many, including myself, run Ganache via the GUI app (not via npm), as it provides a nice interface for inspecting the state of the blockchain. Short of having an extensive testing document that details how it needs to be done, which we don't have, we cannot make assumptions on how people will try to test. Assuming the user is using XCode, the natural inclination will be to do the Command+U to build and run for testing, and not have to revert to the command line to do it.

[yaroslavyaroslav]

1. Persistent state of test mocks are important

I don't think it needs to be persistent, it just needs to be consistent. And what I had before would have satisfied that, as on each run of the tests, Ganache would start at 0, and then the code would add exactly the same 25 blocks to the chain before proceeding with testing, creating the exact same environment for each test session.

Well it's not the same. As you mentioned above class setUp() method calls on each test suit (e.g test class inherited from XCTest) setup proof. So if we're having for example 10 local tests that requires such setup to the end of testing we will have 250 block or something. I think this is redundant. And just to mention we're haven't any native tools except Xcode build phases to add any one time per session setup for testing.

2. Moving blockchain level problem to swift level is not best practice.

This is neither a Swift problem, or a blockchain problem... this is a testing problem. There is nothing wrong with having some code to create a suitable environment for your testing to proceed. In fact it's pretty uncommon not to have some sort of code to initialize/create that environment. Sometimes it's per test, sometimes it's per session.

Yep, if we were able to setup consistent environment per session i would be agree that approach, but as i said in the point above — there's lack of tools to setup environment per [testing] session, not to mention SPM which is providing very limited tool set yet.

[mloit]

Well it's not the same. As you mentioned above class setUp() method calls on each test suit (e.g test class inherited from XCTest) setup proof. So if we're having for example 10 local tests that requires such setup to the end of testing we will have 250 block or something. I think this is redundant. And just to mention we're haven't any native tools except Xcode build phases to add any one time per session setup for testing.

No, because the code as it was written would only create blocks if there were less than 25 blocks on the test chain. So once it ran once, it would early exit on any other calls, and not add more blocks. If it did run, it would only create up to 25 blocks... meaning that if the block count was 0 - it would add 25, if the block count was 5 - it would add 20.

[yaroslavyaroslav]

And how does this work when someone is developing within XCode? using Command+U to build and run tests will no longer work, as it does not start Ganache.

This actually don't works here either. Like every user have to run ganache manually (my thought is by cli) to make any local tests pass.

Can you clarify what you mean by "don't work". If I start Ganache, from the Ganache UI (not from command line) do a "Ethereum Quickstart" And then I can run the tests Command+U, and they do work (Works in XCode for both .xcproj and SPM). Now for this PR, I do need the additional history, because of Oracle - as discussed earlier. My set-up code I had provided exactly that.

This is exact proposed pipeline for manual testing, just reading your that message i haven't found mention about ganache run even if it GUI or CLI, so i agreed with this extended pipeline for manual testing that you provided here.

The user is expected to start Ganache (likely via the GUI), and now they would need to somehow add the necessary number of blocks before the tests can be successfully run.

My assumptions is follow:

* user (developer) will install ganache via npm

* user will run one-liner `ganache   --database.dbPath ./Tests/db.db` instead of `ganache`

Many, including myself, run Ganache via the GUI app (not via npm), as it provides a nice interface for inspecting the state of the blockchain. Short of having an extensive testing document that details how it needs to be done, which we don't have, we cannot make assumptions on how people will try to test. Assuming the user is using XCode, the natural inclination will be to do the Command+U to build and run for testing, and not have to revert to the command line to do it.

I'm agreed that users who uses GUI ganache version will met additional obstacles by this way. And, actually, i'm blaming early stage Ethereum infrastructure for that, but, sorry, that's offtopic, it's just hurts. So despite that, i'm seeing that we're choosing by two options:

• Make test pipeline more convenient and obvious for contributors.
• Make test pipeline more optimized and consistent for pipeline.

As for me at this exact PR i'm about to choose latter, since complex things, mostly like lack of SPM automation tooling, XCTests constrains.

[yaroslavyaroslav]

No, because the code as it was written would only create blocks if there were less than 25 blocks on the test chain. So once it ran once, it would early exit on any other calls, and not add more blocks. If it did run, it would only create up to 25 blocks... meaning that if the block count was 0 - it would add 25, if the block count was 5 - it would add 20.

Oh, I've missed that, this makes it one per session.

[mloit]

So how do we proceed? should I update my other PR to be basically what I had here, were it does a "once per session" run automatically, and only runs if the block count is < 25? You can integrate that one, and hen this one will pass the tests as the infrastructure will be in place then.

[yaroslavyaroslav]

So how do we proceed? should I update my other PR to be basically what I had here, were it does a "once per session" run automatically, and only runs if the block count is < 25? You can integrate that one, and hen this one will pass the tests as the infrastructure will be in place then.

Let's restore it here.