#88, #84, #71, #7 - implemented sending value #91

abdulowork · 2018-03-20T00:24:14Z

@driver733 could you please start with a review. I'll complete missing documentation tomorrow.

#7 - To solve #7 I implemented EthGasPrice for the purpose of asking network for a gas price and EthGasEstimate to estimate the gas needed for most basic value transfer. I added relevant tests.
#84 - I added ganache-cli to travis and redesigned FakeEthereumNetwork to GanacheLocalNetwork which also covers #71. I added some tests with the accounts expected on the ganache testnet.
#88 - I added Account interface and EthAutoAccount which works with network estimates and gas price exclusively. I added an array of missing objects needed for signing and sending transaction to the network.

…k interface and implemented it on existing networks.

…s for GanacheLocalNetwork. Added EthTransactionBytes integration testing.

…h relevant procedures and tests #84 - added another account to ganache

0crat · 2018-03-20T00:24:24Z

Job #91 is now in scope, role is REV

codecov-io · 2018-03-20T00:40:02Z

Codecov Report

Merging #91 into develop will increase coverage by 0.21%.
The diff coverage is 98.01%.

@@             Coverage Diff             @@
##           develop      #91      +/-   ##
===========================================
+ Coverage    96.96%   97.17%   +0.21%     
===========================================
  Files           91      121      +30     
  Lines         2933     3648     +715     
  Branches        26       24       -2     
===========================================
+ Hits          2844     3545     +701     
- Misses          89      103      +14

Impacted Files	Coverage Δ
Example/Actors/Bob.swift	`100% <ø> (ø)`
Web3Swift/RemoteProcedure/ChainIDProcedure.swift	`100% <ø> (ø)`	⬆️
Example/Actors/Alice.swift	`100% <ø> (ø)`
Web3Swift/Transactions/EthTransactions.swift	`100% <100%> (ø)`	⬆️
...e/Tests/TransactionHash/EthTransactionHashIT.swift	`100% <100%> (ø)`
Web3Swift/Transactions/EthTransactionBytes.swift	`100% <100%> (ø)`
Example/Tests/Network/InfuraNetworkTests.swift	`100% <100%> (ø)`	⬆️
Example/Tests/Network/SimpleNetworkTests.swift	`100% <100%> (ø)`	⬆️
Example/Tests/GasPrice/EthGasPriceTests.swift	`100% <100%> (ø)`
Web3Swift/Transaction/EthTransaction.swift	`100% <100%> (ø)`
... and 66 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8e11c2f...f3fbe86. Read the comment docs.

rockfridrich · 2018-03-20T07:52:04Z

Example/Tests/TransactionReceipt/EthTransactionReceiptIT.swift

+    func testExistingTransactionReceipt() {
+        expect{
+            try EthTransactionReceipt(
+                network: InfuraNetwork(


@Biboran Let's use MainnetTestingNetwork instead of InfuraNetwork with metamask params

@rockfridrich sure

rockfridrich · 2018-03-20T07:53:26Z

Example/Tests/Transactions/EthTransactionBytes/EthTransactionBytesTests.swift

+                    )
+                ),
+                recipientAddress: BytesFromHexString(
+                    hex: "0xcD8aC90d9cc7e4c03430d58d2f3e87Dae70b807e"


@Biboran Should we use Alice().privateKey() and Bob().address() in tests?

@rockfridrich there are some problems with this approach. Some of our tests work with mutable state on the blockchain (for example EthAutoAccountIT). If we are going to use Alice and Bob our tests will be temporarily coupled to the greater extent that they already are.

In this particular case I think having the data open is better than introducing any alias objects because it tests low level signing maths

@Biboran original idea was to add Alice and Bob private keys into Ganache initialization script. For some tests, we will need the third character who could be Vitalik

rockfridrich · 2018-03-20T07:57:09Z

Web3Swift/Network/SimpleNetwork.swift

+    - throws:
+    `DescribedError` if something went wrong.
+    */
+    public func id() throws -> NumberScalar {


@Biboran each procedure depends on network and we use procedure in one of the network implementations.

@rockfridrich I believe there is nothing wrong with that.

@Biboran ok, but let's think of introducing some sort of Chain object for this purposes later.

rockfridrich · 2018-03-20T20:21:41Z

Web3Swift/Transactions/EthTransactionBytes.swift

+public final class EthTransactionBytes: BytesScalar {
+
+    private let networkID: NumberScalar
+    private let transactionsCount: NumberScalar


@Biboran should it be nonce?

@rockfridrich lets leave it at transactionsCount as it makes more sense of where the nonce in the transaction is coming from. Transaction has nonce as expected.

0crat · 2018-03-21T15:29:32Z

This pull request #91 is assigned to @driver733 /z, here is why; the budget is 15 minutes, see §4; please, read §27 and when you decide to accept the changes, inform @Biboran /z (the architect) right in this ticket; if you decide that this PR should not be accepted ever, also inform the architect; this blog post will help you understand what is expected from a code reviewerWe should be aware that driver733 is on vacation; this ticket may be delayed

0crat · 2018-03-21T15:29:44Z

Manual assignment of issues is discouraged, see §19: -5 points just awarded to @Biboran /z

rockfridrich · 2018-03-23T09:38:03Z

Web3Swift/Account/EthAutoAccount.swift

+        return try EthBalance(
+            network: network,
+            address: privateKey.address()
+        )


@Biboran should it be .value() here?

@rockfridrich no, it is intended to be lazy

rockfridrich · 2018-03-23T09:41:06Z

Web3Swift/Transactions/EthTransactions.swift

    }

    public func count() throws -> NumberScalar {
-        let transactionsCountProcedure = self.transactionsCountProcedure
+        let transactionsCountProcedure = self.procedure
        return BigEndianCompactNumber(
            hex: SimpleString{
                try transactionsCountProcedure.call()["result"].string()


@Biboran how can we remove ["result"] from the code? Could we create something like .value() as a decorator for JSON?

@rockfridrich we can get rid of ["result"] duplication; however, we cannot make it just .value(). There are different types that can be represented by JSON (string, int etc) and sometimes there is a need to traverse JSON as a dictionary (eth_getTransactionReceipt call).

@rockfridrich if you believe there is an issue with ["result"] duplication make it a separate issue and we will discuss it there

rockfridrich · 2018-03-23T11:59:50Z

Web3Swift/Account/Account.swift

+public protocol Account {
+
+    /**
+    TODO: I am not sure whether the Account should promise to return its balance in wei


@Biboran Probably we should create some sort of value object for Wei that can be easily converted to Eth, GWei and other units like here

0crat · 2018-03-26T15:30:48Z

@driver733 /z this job was assigned to you 5 days ago. It will be taken away from you soon, unless you close it, see §8. Read this and this, please.

0crat · 2018-03-29T15:30:10Z

@driver733 /z this job was assigned to you 8 days ago. It will be taken away from you soon, unless you close it, see §8. Read this and this, please.

0crat · 2018-03-30T15:14:18Z

@driver733 /z this job was assigned to you 9days ago. It will be taken away from you soon, unless you close it, see §8. Read this and this, please.

0crat · 2018-03-30T18:34:54Z

@driver733 /z this job was assigned to you 9days ago. It will be taken away from you soon, unless you close it, see §8. Read this and this, please.

0crat · 2018-03-31T15:31:07Z

The user @driver733 /z resigned from #91, please stop working. Reason for job resignation: It is older than 10 days, see §8

0crat · 2018-03-31T15:31:10Z

Resigned on delay, see §8: -15 point(s) just awarded to @driver733 /z

rockfridrich

Let's merge

0crat · 2018-04-09T06:22:19Z

The job #91 is now out of scope

abdulowork added 14 commits March 14, 2018 18:46

#7 - Added EthGasPrice and tests

6327867

#7 - added EthGasEstimate, EstimateGasProcedure and relevant tests

bd146ce

#7 - Merge branch 'develop' into 7

659e493

#develop - Merge branch '75' into develop

1899dd2

#develop - Merge branch '7' into develop

fba4e7e

#develop - Merge branch '75' into develop

85c1b00

#88 - Added caching decorators for BytesScalar and NumberScalar

31cfff7

#88 - Removed throwing ctors from networks. Added id method to Networ…

9944ba0

…k interface and implemented it on existing networks.

#88 - implemented transaction signing

ff56e76

#88, #86 - Refactored fake network to GanacheLocalNetwork. Added test…

56ca728

…s for GanacheLocalNetwork. Added EthTransactionBytes integration testing.

#86 - Added ganache dependencies

d268783

#86 - update

17c6848

#88 - Updated tests. Added yarn lock

e4f93a2

#88 - Added account, balance, trasaction and its receipt and hash wit…

55159c3

…h relevant procedures and tests #84 - added another account to ganache

0crat added the scope label Mar 20, 2018

Merge branch 'develop' into 88

d19fec8

abdulowork mentioned this pull request Mar 20, 2018

#7 - implemented gas estimation #87

Closed

#88 - Fixed travis

4953768

rockfridrich reviewed Mar 20, 2018

View reviewed changes

abdulowork added 3 commits March 21, 2018 16:22

#88 - corrected buddybuild tests

fc18804

#88 - introduced mainnet alias

19f4410

#88 - added missing documentation

f3fbe86

abdulowork assigned driver733 Mar 21, 2018

rockfridrich reviewed Mar 23, 2018

View reviewed changes

0crat unassigned driver733 Mar 31, 2018

rockfridrich approved these changes Apr 1, 2018

View reviewed changes

abdulowork merged commit 8f410fb into develop Apr 2, 2018

0crat removed the scope label Apr 9, 2018

abdulowork deleted the 88 branch April 13, 2018 09:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#88, #84, #71, #7 - implemented sending value #91

#88, #84, #71, #7 - implemented sending value #91

abdulowork commented Mar 20, 2018

0crat commented Mar 20, 2018

codecov-io commented Mar 20, 2018 •

edited

Loading

rockfridrich Mar 20, 2018

abdulowork Mar 21, 2018

rockfridrich Mar 20, 2018

abdulowork Mar 21, 2018

rockfridrich Mar 21, 2018

rockfridrich Mar 20, 2018

abdulowork Mar 21, 2018

rockfridrich Mar 21, 2018

rockfridrich Mar 20, 2018

abdulowork Mar 21, 2018

0crat commented Mar 21, 2018

0crat commented Mar 21, 2018

rockfridrich Mar 23, 2018

abdulowork Mar 23, 2018

rockfridrich Mar 23, 2018

abdulowork Mar 23, 2018

abdulowork Mar 23, 2018

rockfridrich Mar 23, 2018

0crat commented Mar 26, 2018

0crat commented Mar 29, 2018

0crat commented Mar 30, 2018

0crat commented Mar 30, 2018

0crat commented Mar 31, 2018

0crat commented Mar 31, 2018

rockfridrich left a comment

0crat commented Apr 9, 2018

#88, #84, #71, #7 - implemented sending value #91

#88, #84, #71, #7 - implemented sending value #91

Conversation

abdulowork commented Mar 20, 2018

0crat commented Mar 20, 2018

codecov-io commented Mar 20, 2018 • edited Loading

Codecov Report

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

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

0crat commented Mar 21, 2018

0crat commented Mar 21, 2018

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

0crat commented Mar 26, 2018

0crat commented Mar 29, 2018

0crat commented Mar 30, 2018

0crat commented Mar 30, 2018

0crat commented Mar 31, 2018

0crat commented Mar 31, 2018

rockfridrich left a comment

Choose a reason for hiding this comment

0crat commented Apr 9, 2018

codecov-io commented Mar 20, 2018 •

edited

Loading