Delegated txs #89

szerintedmi · 2018-05-04T18:00:05Z

added AugmintToken.delegatedTransfer() and delegatedTransferAndNotify() see Transferring A-EUR without ETH #84
emit separate Transfer event for transfer fee deduction
added web3 v1 to tests and changed them to use it solely (could make tx signature work with web3 v1 only and truffle uses v0.x)

phraktle · 2018-05-08T20:18:55Z

contracts/generic/AugmintToken.sol

+    /* Transfers based on an offline signed transfer instruction. */
+    function delegatedTransfer(address from, address to, uint amount, string narrative,
+                                     uint minGasPrice,  /* client provided gasPrice on which she expects tx to be exec. */
+                                     uint maxExecutorFee, /* client provided max fee for executing the tx */


specify units of measure (e.g. gas price is in ETH. is fee in ETH or tokens?)

phraktle · 2018-05-08T20:22:41Z

contracts/generic/AugmintToken.sol

+                                     )
+    external returns(bool) {
+
+        require(!noncesUsed[nonce], "nonce already used");


the nonce should be included in a hash along with all other parameters (or just use the signature) and we should store the used hashes (not the nonce). otherwise there's a security issue: I could censor transactions by front-running the nonce from the mem-pool.

also, you could get away with much less bits for the nonce then. you just need to avoid collisions between transactions with the same from/to/amount.

nonce was already included in the hash

pushed a change to store hashes used instead of nonce

lower nonce bits: based on my tests passing+keccak256 of uint32 or bytes4 types actually consume slightly more gas than uint256 or bytes32. Maybe keccak256 is padding each param to same size?

phraktle · 2018-05-08T20:23:35Z

contracts/generic/AugmintToken.sol

+    external returns(bool) {
+
+        require(!noncesUsed[nonce], "nonce already used");
+        require(tx.gasprice >= minGasPrice, "tx.gasprice must be >= minGasPrice");


why do we care about the gas price at this point? the transaction is already being processed...

the idea is that client and "executor" agrees on an executorFee based on a minimum gasPrice. That's what the client signs. This way dishonest executors can't make "extra profit" on submitting tx on lower gas price than the "agreement".

the goal is to minimize the time it takes for my tx to hit the blockchain, not to maximize the profits of miners :) this is not an incentive (or disincentive) for executors.

the only way to ensure that my tx hits the chain in a timely manner is to have some competition between executors. if there's only a single executor for my tx, they may delay my tx for days even if i specified a high gas price. competition would keep them motivated to do their job well.

if an executor was able to have my transaction processed quickly at a lower gas price, they should keep the change. however, if they skimp on the gas too much, competition will beat them to the punch.

(i could also think of more incentives for timely processing, such as expiration dates or rewards diminishing over time.)

as for now do you suggest to get rid of minGasprice ?

phraktle · 2018-05-08T20:24:47Z

contracts/interfaces/AugmintTokenInterface.sol

@@ -26,6 +26,7 @@ contract AugmintTokenInterface is Restricted, ERC20Interface {
    mapping(address => mapping (address => uint256)) public allowed; // allowances added with approve()

    TransferFeeInterface public feeAccount;
+    mapping(bytes32 => bool) public noncesUsed; // record nonces used by delegatedTransfer


see a prior comment (store tx hashes, not nonces)

phraktle · 2018-05-08T20:30:00Z

contracts/generic/AugmintToken.sol

+                                     )
+    external returns(bool) {
+
+        require(!noncesUsed[nonce], "nonce already used");


also, you could get away with much less bits for the nonce then. you just need to avoid collisions between transactions with the same from/to/amount.

phraktle · 2018-05-09T09:43:14Z

contracts/generic/AugmintToken.sol

+    external returns(bool) {
+
+        require(!noncesUsed[nonce], "nonce already used");
+        require(tx.gasprice >= minGasPrice, "tx.gasprice must be >= minGasPrice");


the goal is to minimize the time it takes for my tx to hit the blockchain, not to maximize the profits of miners :) this is not an incentive (or disincentive) for executors.

the only way to ensure that my tx hits the chain in a timely manner is to have some competition between executors. if there's only a single executor for my tx, they may delay my tx for days even if i specified a high gas price. competition would keep them motivated to do their job well.

if an executor was able to have my transaction processed quickly at a lower gas price, they should keep the change. however, if they skimp on the gas too much, competition will beat them to the punch.

(i could also think of more incentives for timely processing, such as expiration dates or rewards diminishing over time.)

phraktle · 2018-05-11T20:23:39Z

contracts/generic/AugmintToken.sol

+
+        bytes32 txHash = keccak256(this, from, target, amount, data, minGasPrice, maxExecutorFeeInToken, nonce);
+
+        require(!delegatedTxHashesUsed[txHash], "txHash already used");


suggest factoring common code between the two delegated... functions.

szerintedmi added 3 commits May 4, 2018 16:17

fix timing issues with collect loan tests

70910c4

use web3v1 in tests (prep for tx signature test, requires web3v1)

c3455c2

first pass impl. for delegatedTransfer - #84

c920db8

szerintedmi requested a review from phraktle May 5, 2018 09:15

AugmintToken tests catch up with new TxDelegator

186178f

szerintedmi changed the title ~~Delegate txs~~ Delegated txs May 5, 2018

szerintedmi mentioned this pull request May 5, 2018

Transferring A-EUR without ETH #84

Closed

szerintedmi added 4 commits May 8, 2018 15:46

no need to deploy SafeMath it's part of each contract using it

f546913

order of migration steps - deploy InterestEarnedAcc before TokenAEur

20857c8

move recover fx to a ECRecovery lib

168e75b

get rid of sep. TxDelegator, fx in AugmintToken + ECRecovery lib

ac15a74

szerintedmi force-pushed the delegate_txs branch from eaf2554 to ac15a74 Compare May 8, 2018 19:04

szerintedmi added 3 commits May 8, 2018 21:12

minor test assertion fix in fee calc for NoFeeTransferContracts

73fa014

balance tests for delegatedTransfers + nonce test

a2f9582

set AugmintTransfer event to & from indexed (restore original)

2cacd05

phraktle requested changes May 8, 2018

View reviewed changes

szerintedmi added 5 commits May 8, 2018 21:30

delegatedTransfer return no value (reverts on fail) + narrative change

4bde61b

store delegated tx hashes used instead of nonce

798263a

units specifed in delegatedTransfer arg names

d9d4781

0 transfer fee on executorFee transfer

1a7c624

finish delegatedTransfer tests

e36caf2

phraktle requested changes May 9, 2018

View reviewed changes

szerintedmi added 3 commits May 9, 2018 13:17

delegatedTransferAndNotify + tests

f2ad542

new tokenAEur ABI & local deployment addresses

d35e1f7

deploy a set of legacy contracts for local testing new augminTokens

2669e08

szerintedmi force-pushed the delegate_txs branch from 22ead4e to 2669e08 Compare May 10, 2018 11:12

adjust test values to changed migration data

d08f8ea

phraktle requested changes May 11, 2018

View reviewed changes

got rid of minGasPrice from delegatedTransfer* fxs

9d6de9a

szerintedmi added 5 commits May 11, 2018 17:18

delegatedTransfer* fx common parts into private fx

58634d6

emit ERC20 transfer for fee part of transfer too.

574350d

update ganache abi & deploy info

7241223

deploy new TokenAEur and all dependent contracts to Rinkeby

3620d07

update scripts how they should have been ran

b5f6106

szerintedmi merged commit a2f6b11 into staging May 12, 2018

szerintedmi deleted the delegate_txs branch May 23, 2018 22:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delegated txs #89

Delegated txs #89

szerintedmi commented May 4, 2018 •

edited

Loading

phraktle May 8, 2018 •

edited

Loading

phraktle May 8, 2018

phraktle May 8, 2018

szerintedmi May 8, 2018 •

edited

Loading

phraktle May 8, 2018

szerintedmi May 8, 2018

phraktle May 9, 2018

szerintedmi May 11, 2018

phraktle May 11, 2018

phraktle May 8, 2018

szerintedmi May 8, 2018

phraktle May 8, 2018

phraktle May 9, 2018

phraktle May 11, 2018


		bytes32 txHash = keccak256(this, from, target, amount, data, minGasPrice, maxExecutorFeeInToken, nonce);

		require(!delegatedTxHashesUsed[txHash], "txHash already used");

Delegated txs #89

Delegated txs #89

Conversation

szerintedmi commented May 4, 2018 • edited Loading

phraktle May 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szerintedmi May 8, 2018 • 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

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

szerintedmi commented May 4, 2018 •

edited

Loading

phraktle May 8, 2018 •

edited

Loading

szerintedmi May 8, 2018 •

edited

Loading