Use Option for CallContract member 'data' #1076

tcharding · 2019-07-10T08:46:36Z

Currently we use bytes.default() to generate an empty value for byte array data member of the CallContract structure. This results in the value 0x being used (I do not know the exact reason why or where this 0x is found, please see note below on testing). Rust has options for such a case.

This needs testing, I have no idea what will now be expressed by the None in place of the original 0x.

Fixes: #999

thomaseizinger · 2019-07-11T01:31:55Z

CallContract gets converted to ActionResponseBody::EthereumCallContract.

What do you think @D4nte and @luckysori in terms of the API?
I think we want a test that the data field just does not exist if we serialize ActionResponseBody::EthereumCallContract to JSON and the Option is None.

D4nte · 2019-07-11T02:04:20Z

CallContract gets converted to ActionResponseBody::EthereumCallContract.

What do you think @D4nte and @luckysori in terms of the API?
I think we want a test that the data field just does not exist if we serialize ActionResponseBody::EthereumCallContract to JSON and the Option is None.

Yes a serialisation test when data is None to confirm it's not present in the JSON would be great please.

thomaseizinger · 2019-07-11T02:26:39Z

CallContract gets converted to ActionResponseBody::EthereumCallContract.
What do you think @D4nte and @luckysori in terms of the API?
I think we want a test that the data field just does not exist if we serialize ActionResponseBody::EthereumCallContract to JSON and the Option is None.

Yes a serialisation test when data is None to confirm it's not present in the JSON would be great please.

@tcharding Go for that then :)

tcharding · 2019-07-12T04:19:23Z

The enum variant ActionResponseBody::EthereumCallContract uses a struct that has fields that mirror the ethereum::CallContract struct. This is duplication of knowledge, we can just use the ethereum::CallContract directly in the enum variant instead of a struct. This allows us to rely on the serialization testing done for CallContract. The bitcoin variants are probably able to be done the same way but its Friday afternoon so this is all for this PR :) Can be merged without doing the bitcoin variants IMO.

D4nte

Great insight. Well done.

thomaseizinger

The enum variant ActionResponseBody::EthereumCallContract uses a struct that has fields that mirror the ethereum::CallContract struct.

Sorry but this is actually very much on purpose!

IMO the CallContract and DeployContract structs are part of the COMIT core (we should soon start extracting that into its own crate to make that one clear). Hence, they shouldn't implement Serialize/Deserialize in order to not leak their inner structure. (Even if you were to implement Serialize, you should probably derive it in order to not having to write the implementation manually)
For Bitcoin, the data is actually different because there is a signing step that happens between emitting the action from the state and serializing it to the user.

For consistency reasons, I'd therefore not re-use those structs there. IMO that causes more confusion than it adds value in terms of de-duplication. It will look like a job half done which cannot be finished because what was done for Ethereum cannot be done for Bitcoin.

I'd say this applies in general. Separate data structures for any external facing communication is key if you want to avoid accidental breaking API changes through actions like renamed fields.

In order to fully embrace that (it is not enforced currently), we probably should do something like this:

EthereumCallContract {
    contract_address: Http<ethereum_support::Address>,
    data: Http<ethereum_support::Bytes>,
    gas_limit: Http<ethereum_support::U256>,
    network: Http<ethereum_support::Network>,
    min_block_timestamp: Option<Http<Timestamp>>,
},

(Note that all types are wrapped in Http). This would allow us to actually pin down the API and would increase awareness about breaking API changes since you have to actively change the impl Serialize for Http<ethereum_support::U256> for example.

D4nte · 2019-07-15T02:48:14Z

Taking over, I will just remove commit 601aa41

comit_node/src/swap_protocols/actions.rs

da-kami

looks legit :)

bonomat

I'm just blocking this as we have two approvals and we should not accidentally get the flaky test merged into master

CLAassistant · 2019-07-18T00:25:52Z

All committers have signed the CLA.

Currently we use bytes.default() to generate an empty value for bytes array `data` in the `CallContract` structure. This results in the value `0x` being used (I do not know the exact reason). Rust has options for such a case. This needs testing, I have no idea what will now be expressed by the `None` in place of the original `0x` (FTR I do not know where the `Ox` is either :) Fixes: #999

`CallContract` has some `Option<T>` fields. When `CallContract` is serialized we would like field members that have the value `None` to be _not_ in the JSON string. Add failing unit test for serialization of `CallContract` with `None` valued fields.

`CallContract` has some `Option<T>` fields. When `CallContract` is serialized we would like field members that have the value `None` to be _not_ in the JSON string. Make failing unit test for serialization of `CallContract` with `None` valued fields pass.

thomaseizinger

Good work Franck! 🎉

thomaseizinger · 2019-07-18T00:52:04Z

api_tests/lib/ethereum.ts

@@ -147,7 +147,7 @@ export class EthereumWallet {

    public async sendEthTransactionTo(
        to: string,
-        data: string = "0x0",


This stupid EthereumWallet has bitten us so many times now, on bobtimus as well as here in the tests.
I think we should extract what we have here into an NPM package and publish it. Basically an idiot-proof EthereumWallet that cannot be used wrongly :D

hahahahaha, yes I think it's worth considering and to give back to the community.

Created ticket here: coblox/bobtimus#78

bonomat

Good job. That definitely was an easy-first-ticket :D

tcharding assigned D4nte, thomaseizinger and luckysori Jul 10, 2019

tcharding added automagically-mergify-this and removed automagically-mergify-this labels Jul 11, 2019

tcharding assigned tcharding and unassigned D4nte, thomaseizinger and luckysori Jul 11, 2019

tcharding force-pushed the 999-eth-refund-action branch 2 times, most recently from 7f87858 to 601aa41 Compare July 12, 2019 04:13

tcharding marked this pull request as ready for review July 12, 2019 04:15

tcharding added the automagically-mergify-this label Jul 12, 2019

tcharding requested review from thomaseizinger, D4nte and luckysori July 12, 2019 04:19

D4nte approved these changes Jul 12, 2019

View reviewed changes

thomaseizinger suggested changes Jul 12, 2019

View reviewed changes

thomaseizinger assigned D4nte Jul 15, 2019

D4nte force-pushed the 999-eth-refund-action branch from 601aa41 to 41c6f7b Compare July 15, 2019 02:49

D4nte requested a review from thomaseizinger July 15, 2019 02:49

D4nte unassigned tcharding Jul 15, 2019

thomaseizinger suggested changes Jul 15, 2019

View reviewed changes

comit_node/src/swap_protocols/actions.rs Outdated Show resolved Hide resolved

comit_node/src/swap_protocols/actions.rs Outdated Show resolved Hide resolved

D4nte removed the automagically-mergify-this label Jul 15, 2019

D4nte requested a review from thomaseizinger July 15, 2019 04:54

D4nte added the automagically-mergify-this label Jul 15, 2019

D4nte requested a review from da-kami July 15, 2019 04:54

da-kami approved these changes Jul 15, 2019

View reviewed changes

thomaseizinger approved these changes Jul 15, 2019

View reviewed changes

D4nte force-pushed the 999-eth-refund-action branch from be1387f to 929e9db Compare July 15, 2019 06:08

bonomat suggested changes Jul 17, 2019

View reviewed changes

bonomat removed the automagically-mergify-this label Jul 17, 2019

D4nte requested a review from thomaseizinger July 18, 2019 00:35

This comment has been minimized.

Sign in to view

D4nte changed the title ~~Use Option for CallContract member 'data'~~ [Do Not Merge] Use Option for CallContract member 'data' Jul 18, 2019

Tobin C. Harding and others added 7 commits July 18, 2019 10:39

Skip serialization of none values for the http api action payload

aa4fa0c

min_block_timestamp is not encoded if null

a4b392f

data is not encoded if null

940c153

e2e: Should not default Ethereum transaction data to "0x0"

708aa89

D4nte force-pushed the 999-eth-refund-action branch from 84e3a5f to 708aa89 Compare July 18, 2019 00:41

D4nte changed the title ~~[Do Not Merge] Use Option for CallContract member 'data'~~ Use Option for CallContract member 'data' Jul 18, 2019

D4nte requested review from bonomat, da-kami and D4nte July 18, 2019 00:48

thomaseizinger approved these changes Jul 18, 2019

View reviewed changes

bonomat approved these changes Jul 18, 2019

View reviewed changes

D4nte merged commit 81a1fa4 into master Jul 18, 2019

D4nte deleted the 999-eth-refund-action branch July 18, 2019 01:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Option for CallContract member 'data' #1076

Use Option for CallContract member 'data' #1076

tcharding commented Jul 10, 2019

thomaseizinger commented Jul 11, 2019

D4nte commented Jul 11, 2019

thomaseizinger commented Jul 11, 2019

tcharding commented Jul 12, 2019

D4nte left a comment

thomaseizinger left a comment

D4nte commented Jul 15, 2019

da-kami left a comment

bonomat left a comment

CLAassistant commented Jul 18, 2019 •

edited

This comment has been minimized.

thomaseizinger left a comment

thomaseizinger Jul 18, 2019

D4nte Jul 18, 2019

thomaseizinger Jul 18, 2019

bonomat left a comment

Use Option for CallContract member 'data' #1076

Use Option for CallContract member 'data' #1076

Conversation

tcharding commented Jul 10, 2019

thomaseizinger commented Jul 11, 2019

D4nte commented Jul 11, 2019

thomaseizinger commented Jul 11, 2019

tcharding commented Jul 12, 2019

D4nte left a comment

Choose a reason for hiding this comment

thomaseizinger left a comment

Choose a reason for hiding this comment

D4nte commented Jul 15, 2019

da-kami left a comment

Choose a reason for hiding this comment

bonomat left a comment

Choose a reason for hiding this comment

CLAassistant commented Jul 18, 2019 • edited

This comment has been minimized.

thomaseizinger left a comment

Choose a reason for hiding this comment

thomaseizinger Jul 18, 2019

Choose a reason for hiding this comment

D4nte Jul 18, 2019

Choose a reason for hiding this comment

thomaseizinger Jul 18, 2019

Choose a reason for hiding this comment

bonomat left a comment

Choose a reason for hiding this comment

CLAassistant commented Jul 18, 2019 •

edited