Update Expected Method handling

[mojoX911]

Fixes #21 , Tried few options, it seems the simplest thing to do for now is to use an ExpectedMessage enum instead of &str.

if let probably doesn't make much sense here, because some kind of matching has to happen somewhere. And we cant do it outside handle_message() also as it needs wallet and rpc links.

Using message mapping if lets ends up having same amount of code, and I don't think it improves any performance.

Updated the name "method" to "messages" as it makes more intuitive sense to me and they mean the same thing anyway.

If anything more that can be improved, let me know.

[codecov-commenter]

Codecov Report

Merging #23 (7494f78) into master (237c8ae) will decrease coverage by 0.70%. The diff coverage is 49.18%.

@@            Coverage Diff             @@
##           master      #23      +/-   ##
==========================================
- Coverage   81.60%   80.89%   -0.71%     
==========================================
  Files           8        8              
  Lines        2571     2586      +15     
==========================================
- Hits         2098     2092       -6     
- Misses        473      494      +21     

Impacted Files	Coverage Δ
src/maker_protocol.rs	80.77% <49.18%> (-4.90%)	:arrow_down:

---

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 237c8ae...7494f78. Read the comment docs.

[chris-belcher]

This way of doing it doesn't fix the issue #21, as it still relies on strings for which there is no compiler checking.

[mojoX911]

It still has to use string because we are encoding messages using serde here

https://github.com/bitcoin-teleport/teleport-transactions/blob/9ddd6db88d890146d301385dd4cb9be10ea510ef/src/messages.rs#L102-L104

Every message has a method tag, which is lowercase string name of the enum variant.

Which is then converted here

https://github.com/mojoX911/teleport-transactions/blob/62ceca6a191de70be675e41be98ccc0fbd2056bb/src/maker_protocol.rs#L271-L272

Because this string is generated from serde we can expect it will always have string from a specific set (name of enum variants). I am not sure what other assurance we can get unless we change the encoding itself, as discussed here https://github.com/bitcoin-teleport/teleport-transactions/issues/19.

If we are talking about removing string conversion from messages, we basically need to have other encoding methods, which is beyond the scope of this PR.

In Issue #21 I thought the intend was to get rid of the control list as a Vec<&str> into something more concrete.

I think this PR can stand on its own, as irrespective of how we encode stuffs its better to have an enum here than a list of strings as allowed methods.

The total removal of strings in message method field can happen later depending on what we decide in #19

[chris-belcher]

There was a misunderstanding. String comparisons are not bad by themselves, the bad thing is using string comparisions instead of enum comparisons, because enum comparisons are subject to checking by the rust compiler.

Removing string entirely as suggested in #19 is missing the point. If you used bytes (e.g. 0x01, 0x02, etc) instead then there still would not be compiler checking, because typos such as 0xe1 would not be detected by the compiler.

I imagine the maker main loop to work where it has an enum that defines which messages it will accept. The main loop would be a big match statement over this enum, and within each branch the individual messages would be parsed with if let.

Here is some pseudocode for how I imagine the maker main loop to work without string comparisons (or rather, with string comparisons only inside the serde crate)

enum AcceptedMessage {
    TakerHello,
    NewlyConnectedTaker,
    SignSendersContractTx,
    ProofOfFunding,
    SendersAndReceiversContractSigs,
    SignReceivedContractTx,
    HashPreimage,
    PrivateKeyHandover,
}

struct ConnectionState {
    accepted_message: AcceptedMessage
    // other fields here
}

async fn run(rpc: Arc<Client>, wallet: Arc<RwLock<Wallet>>, port: u16) -> Result<(), Error> {

    // set up the TcpListener socket

    loop {
    // accept incoming connections, check for errors

    tokio::spawn(async move {
        let mut connection_state = ConnectionState {
            accepted_message: AcceptedMessage::TakerHello,
            // other fields here
        };

        // handshake with the client

        loop {
            // read data from the client socket

            let message_result = handle_message(
                line,
                &mut connection_state,
                Arc::clone(&client_rpc),
                Arc::clone(&client_wallet),
            );

            // handle message result
        }
    }
    }
}

fn handle_message(
    line: String,
    connection_state: &mut ConnectionState,
    rpc: Arc<Client>,
    wallet: Arc<RwLock<Wallet>>,
) -> Result<Option<MakerToTakerMessage>, Error> {

    let request: TakerToMakerMessage = match serde_json::from_str(&line) {
    Ok(r) => r,
    Err(_e) => return Err(Error::Protocol("message parsing error")),
    };
    log::trace!(target: "maker", "<== {:?}", request);

    let outgoing_message = match connection_state.accepted_message {
    AcceptedMessage::TakerHello => {
        let message = if let TakerToMakerMessage::TakerHello(m) = request {
            m
        } else {
            return Err(Error::Protocol("expected takerhello message"));
        }
        connection_state.accepted_message = AcceptedMessage::NewlyConnectedTaker;
        None
    }
    AcceptedMessage::NewlyConnectedTaker => {
        match request {
            TakerToMakerMessage::GiveOffer(_message) => {
                // handle giveoffer
                connection_state.accepted_message = AcceptedMessage::SignSendersContractTx
            }
            TakerToMakerMessage::SignSendersContractTx(_message) => {
                // handle signsenderscontracttx and set connection_state.accepted_message
            }
            TakerToMakerMessage::ProofOfFunding(_message) => {
                // handle proofoffunding and set connection_state.accepted_message
            }
            TakerToMakerMessage::SignReceiversContractTx(message) => {
                // handle signreceiverscontracttx and set connection_state.accepted_message
            }
            TakerToMakerMessage::HashPreimage(message) => {
                // handle hashpreimage and set connection_state.accepted_message
            }
            _ => {
                return Err(Error::Protocol("unexpected method"));
            }
        }
    }
    AcceptedMessage::SignSendersContractTx => {
        let message = if let TakerToMakerMessage::SignSendersContractTx(m) = request {
            m
        } else {
            return Err(Error::Protocol("expected signsenderscontracttx message"));
        }
        connection_state.accepted_message = AcceptedMessage::ProofOfFunding;
        handle_sign_senders_contract_tx(wallet, message)?
    }
    AcceptedMessage::ProofOfFunding => {
        let proof = if let TakerToMakerMessage::ProofOfFunding(m) = request {
            m
        } else {
            return Err(Error::Protocol("expected proofoffunding message"));
        }
        connection_state.accepted_message = AcceptedMessage::SendersAndReceiversContractSigs;
        handle_proof_of_funding(connection_state, rpc, wallet, &proof)?
    }
    // handle all the other cases here
    }

    // do other stuff then return
}

A few points:

• the main loop has a big match pattern which handles all the enum AcceptedMessage cases
• inside most of those cases the if let pattern is used
• except for AcceptedMessage::NewlyConnectedTaker where there is another match pattern which checks for the allowed messages, and uses the placeholder _ to return an error

[mojoX911]

Ah ok, thanks for the detailed explanation. I did misunderstood the objective.

Have updated the PR to address the same.

To reiterate my understanding:

• Instead checking the method of received message, we will simply decompose the message and check if its matches expectation.

• This will allow us to remove the method variable in front of TakerToMakerMessage all together.

• We wont require string (or byte) comparison anymore.

• If we also have a NewlyConnectedTaker variant of ExpectedMessage then we can remove Option<ExpectedMessage> in ConnectionState.

  Rewriting the previous commit as it will make the review easier.

  Rebased on current master.

[chris-belcher]

Thanks for the update. I've added some comments and style.

Your understanding is fine I think.

Except for this:

This will allow us to remove the method variable in front of TakerToMakerMessage all together.

I think the serde crate might require a method variable, because otherwise it doesn't know which struct to deserialize a message into. That's okay from our point of view because we still only deal with enums, as long as the string (or byte) comparison happens inside serde and not in our code.

[mojoX911]

Thanks for the comments.

Updated accordingly.

Just to clarify my understanding

I think the serde crate might require a method variable, because otherwise it doesn't know which struct to deserialize a message into. That's okay from our point of view because we still only deal with enums, as long as the string (or byte) comparison happens inside serde and not in our code.

If we move out from serde serialization to something else then we won't need this method flags anymore right?

[mojoX911]

If we move out from serde serialization to something else then we won't need this method flags anymore right?

On further thought it seems because TakerToMakerMessage is an enum we would need some flag to deserialise the right variant. So my comment wasn't correct.