[ContractKit]Fill more fields before web3 signing

[ashishb]

Description

1. Fill more fields before web3 signing of transaction.
2. Treat '0' as an empty value

Tested

Manually checked that the new methods work by looking at debug logs from DEBUG=* jest src/utils/tx-signing.test.ts -t test

Issues

Partially addresses #526

[codecov]

Codecov Report

Merging #1133 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1133   +/-   ##
=======================================
  Coverage   66.16%   66.16%           
=======================================
  Files         261      261           
  Lines        7599     7599           
  Branches      508      508           
=======================================
  Hits         5028     5028           
  Misses       2474     2474           
  Partials       97       97

Flag	Coverage Δ
#mobile	66.16% <ø> (ø)	⬆️

---

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 b53f2d0...d1283c8. Read the comment docs.

[mcortesi]

@ashishb Let's go back to the drawing board here...
There a couple of confusing things:

Things to note:

• kit.sendTransaction()
  set gasPrice = 0 by default. Doesn't not attempts to estimateGas by itself. -
• kit.sendTransactionObject() Estimate gas if no estimate is given. Does not consider gasCurrency for that. Fix (always) gasPrice to 0 (no matter what the user wants)

And now we are doing similar things on the provider.

We should define where we should do "what" and be consitent about it. Need to think about it, but let's talk if you want

[mcortesi]

@ashishb Let's go back to the drawing board here...
There a couple of confusing things:

Things to note:

• kit.sendTransaction()
  set gasPrice = 0 by default. Doesn't not attempts to estimateGas by itself. -
• kit.sendTransactionObject() Estimate gas if no estimate is given. Does not consider gasCurrency for that. Fix (always) gasPrice to 0 (no matter what the user wants)

And now we are doing similar things on the provider.

We should define where we should do "what" and be consitent about it. Need to think about it, but let's talk if you want

Following on this.

1. It's alright not to consider gasCurrency on estimateGas
2. we should make sendTransaction estimateGas the same way sendTransactionObject does.
3. both function should allow user to define their gasPrice if they want, and if not, set gasPrice=0 to signal the node to compute it
4. The provider should not attempt to estimate gas. If gas is not present then fail. That's the user or web3 responsability.
5. The provider is gasPrice == 0 should attempt to compute gasPrice considering gasCurrency. We can for now fail if gasCurrency != Gold and handle that case sometime in the future. If the use wants to pay with cUsd it should set it's own gasPrice.

I favor this last fail step, since to access kit from the provider and thus create this circular dependency is a step i would like to analyze further and don't take so lightly.

[ashishb]

both function should allow user to define their gasPrice if they want, and if not, set gasPrice=0 to signal the node to compute it

That, as we know, only works when geth node is getting unsigned txns.

The provider should not attempt to estimate gas. If gas is not present then fail. That's the user or web3 responsability.

Why not? Provider seems like the right abstraction level to do this. So, if someone uses web3 instead of using kit.sendTransaction, it would still work.

The provider is gasPrice == 0 should attempt to compute gasPrice considering gasCurrency. We can for now fail if gasCurrency != Gold and handle that case sometime in the future. If the use wants to pay with cUsd it should set it's own gasPrice.

I modified the code, now it takes an optional GasPriceMinimum contract. If that param is provided, it can handle cUSD. If that param is not provided, it will fail with error for gas currency ≠ null (or undefined).

[mcortesi]

both function should allow user to define their gasPrice if they want, and if not, set gasPrice=0 to signal the node to compute it

That, as we know, only works when geth node is getting unsigned txns.

I know; but we should have a clear definition of objects responsibilities and expectations

web3/kit: They should be able to assume they are speaking directly with a node. No local signing. So, from their perspective setting gasPrice=0 is ok. That's the abstraction they manage. If not; they need to be asking themselves "is the provider local o remote?" when there's not such thing. And we are breaking the abstraction that way. having a leaky one.

Provider A provider should behave the same way as a node. It shouldn't be any difference between talking to a provider that does local signing, than talking to a provider that relays all message to a node

Because of this; imo, we should set gasPrice=0 since we know that the node will fill the gasPrice themselves.

The provider should not attempt to estimate gas. If gas is not present then fail. That's the user or web3 responsability.

Why not? Provider seems like the right abstraction level to do this. So, if someone uses web3 instead of using kit.sendTransaction, it would still work.

The question here is: how does the node behave when there's no gasLimit set? I understand that it should; but we should check.

My thinking goes this way: If the node doesn't automatically set the gasLimit for us, then our provider shouldn't attempt to do so.

The provider is gasPrice == 0 should attempt to compute gasPrice considering gasCurrency. We can for now fail if gasCurrency != Gold and handle that case sometime in the future. If the use wants to pay with cUsd it should set it's own gasPrice.

I modified the code, now it takes an optional GasPriceMinimum contract. If that param is provided, it can handle cUSD. If that param is not provided, it will fail with error for gas currency ≠ null (or undefined).

I'll check this on the code

[mcortesi]

The provider should not attempt to estimate gas. If gas is not present then fail. That's the user or web3 responsability.

Why not? Provider seems like the right abstraction level to do this. So, if someone uses web3 instead of using kit.sendTransaction, it would still work.

The question here is: how does the node behave when there's no gasLimit set? I understand that it should; but we should check.

My thinking goes this way: If the node doesn't automatically set the gasLimit for us, then our provider shouldn't attempt to do so.

https://github.com/ethereum/wiki/wiki/JSON-RPC#eth_sendtransaction
Following on this... the docs says the it's optional, and the default value is 90000. I don't know if the doc is updated.