Create better way of controlling acceptable protocol methods

[chris-belcher]

Right now the maker server handles protocol messages by having a big match statement that handles each message:

teleport-transactions/src/maker_protocol.rs

Lines 233 to 273 in 4282fd6

	let outgoing_message = match request {
	TakerToMakerMessage::TakerHello(_message) => {
	connection_state.allowed_method = None;
	None
	}
	TakerToMakerMessage::GiveOffer(_message) => {
	let max_size = wallet.read().unwrap().get_offer_maxsize(rpc)?;
	let tweakable_point = wallet.read().unwrap().get_tweakable_keypair().1;
	connection_state.allowed_method = Some("signsenderscontracttx");
	Some(MakerToTakerMessage::Offer(Offer {
	absolute_fee: 1000,
	amount_relative_fee: 0.005,
	max_size,
	min_size: 10000,
	tweakable_point,
	}))
	}
	TakerToMakerMessage::SignSendersContractTx(message) => {
	connection_state.allowed_method = Some("proofoffunding");
	handle_sign_senders_contract_tx(wallet, message)?
	}
	TakerToMakerMessage::ProofOfFunding(proof) => {
	connection_state.allowed_method = Some("sendersandreceiverscontractsigs");
	handle_proof_of_funding(connection_state, rpc, wallet, &proof)?
	}
	TakerToMakerMessage::SendersAndReceiversContractSigs(message) => {
	connection_state.allowed_method = Some("signreceiverscontracttx");
	handle_senders_and_receivers_contract_sigs(connection_state, rpc, wallet, message)?
	}
	TakerToMakerMessage::SignReceiversContractTx(message) => {
	connection_state.allowed_method = Some("hashpreimage");
	handle_sign_receivers_contract_tx(wallet, message)?
	}
	TakerToMakerMessage::HashPreimage(message) => {
	connection_state.allowed_method = Some("privatekeyhandover");
	handle_hash_preimage(wallet, message)?
	}
	TakerToMakerMessage::PrivateKeyHandover(message) => {
	handle_private_key_handover(wallet, message)?
	}
	};

However a taker can't validly send any message at any time, they have to follow a specific sequences of messages. So after the fact while coding I added code such as connection_state.allowed_method which checks that the protocol message is allowed e.g.

teleport-transactions/src/maker_protocol.rs

Lines 210 to 226 in 4282fd6

	let request_json: Value = match serde_json::from_str(&line) {
	Ok(r) => r,
	Err(_e) => return Err(Error::Protocol("json parsing error")),
	};
	let method = match request_json["method"].as_str() {
	Some(m) => m,
	None => return Err(Error::Protocol("missing method")),
	};
	let is_method_allowed = match connection_state.allowed_method {
	Some(allowed_method) => method == allowed_method,
	None => NEWLY_CONNECTED_TAKER_ALLOWED_METHODS
	.iter()
	.any(|&r| r == method),
	};
	if !is_method_allowed {
	return Err(Error::Protocol("unexpected method"));
	}

and

teleport-transactions/src/maker_protocol.rs

Lines 34 to 46 in 4282fd6

	//TODO
	//this using of strings to indicate allowed methods doesnt fit aesthetically
	//with the using of structs and serde as in messages.rs
	//there is also no additional checking by the compiler
	//ideally this array and the other strings in this file would instead be
	//structs, however i havent had time to figure out if rust can do this
	pub const NEWLY_CONNECTED_TAKER_ALLOWED_METHODS: [&str; 5] = [
	"giveoffer",
	"signsenderscontracttx",
	"proofoffunding",
	"signreceiverscontracttx",
	"hashpreimage",
	];

However this is based on string comparisons, and is very un-rust-like. There is no checking by the compiler. A typo or other error would have to be detected at runtime instead of compile-time. We should be able to do a lot better.

I originally thought there could be some kind of list of structs which could control which enum is allowed to be deserialized, but I think that cant be done in rust. I was reading again the rust book: https://doc.rust-lang.org/book/ch06-00-enums.html and I think the only way to deal with the message enums and structs is to use match and if let.

One possible way of recoding the maker to improve this situation is to remove the big match statement that handles each message, and have the maker server be a state machine. If the state is waiting for one specific protocol message (for example TakerHello, SendersAndReceiversContractSigs or PrivateKeyHandover) then the code can use if let to check that the protocol message really is that message, and if not then create an error. If the state of the server is waiting for multiple possible protocol messages (for example GiveOffer, ProofOfFunding or HashPreimage) then there can be a match statement handling only those acceptable messages, and if something else arrives then create an error.

Other ways might be possible too.

[chris-belcher]

@mojoX911 I suggest you look at this and the other issues I opened today (for some reason I can't assign you on github)

[jkczyz]

Using a state machine may be ideal. See the following section in the rust book for the rustic way of implementing the pattern, which differs from the typical OO approach.

https://doc.rust-lang.org/book/ch17-03-oo-design-patterns.html#encoding-states-and-behavior-as-types

[mojoX911]

Hi sorry for the delay, was stuck in other part of the world.

Yes I noticed this part is using strings, but ideally it should use some form of enum.

I originally thought there could be some kind of list of structs which could control which enum is allowed to be deserialized, but I think that cant be done in rust. I was reading again the rust book: https://doc.rust-lang.org/book/ch06-00-enums.html and I think the only way to deal with the message enums and structs is to use match and if let.

I think the enum with if let is the only possible pattern match here in rust, and that does serve the purpose. We might not have to explicitly define what message is allowed when, but with pattern matching we can ensure in compile time that only that was allowed is what received.

I am not sure if the problem is more broader than that.

I will look into this parts and see what I find.

thanks @jkczyz That also something we can do. We can say the makers maintains some state, and depending on the state different messages would be accepted. The state can be simple enum. And then we can merge the state and message combinations inside the message handling logic to handle each message while ensuring it has the correct state. I dont think we should need anything more fancy than enum with if lets to do that.

I will try to cook up some simple cases.

[mojoX911]

It seems I cannot self assign myself too. No issues..