feat(xcvm): simplify apply_bindings function

Rather than dealing with pre-initialised buffer and having to track
indexes when setting data in the output payload, change the
apply_bindings function to append data into a vector which does index
tracking by itself.  This removes offset tracking from the function
simplifying it.  With that, change the function so it no longer takes
output vector as argument but rather allocates vector internally.

While changing the apply_bindings function in xc_core also change how
it’s used in the interpreter contract refactoring the code slightly.
Part of it is a consequence of changing the signature of the
apply_binding function but partially it’s just refactoring splitting
functions into smaller, more manageable chunks.

code/xcvm/cosmwasm/contracts/interpreter/src/contract.rs

@@ -243,78 +243,105 @@ pub fn interpret_call(
 	deps: Deps,
 	env: &Env,
 	bindings: Vec<(u32, BindingValue)>,
-	payload: Vec<u8>,
+	mut payload: Vec<u8>,
 	instruction_pointer: u16,
 	tip: &Addr,
 ) -> Result {
-	// we hacky using json, but we always know ABI encoding dependng on chain we run on send to
-	let flat_cosmos_msg: xc_core::cosmwasm::FlatCosmosMsg<serde_cw_value::Value> = if !bindings
-		.is_empty()
-	{
+	if !bindings.is_empty() {
+		let resolver = BindingResolver::new(&deps, env, instruction_pointer, tip)?;
+		let p = core::mem::take(&mut payload);
+		payload = apply_bindings(p, &bindings, |binding| resolver.resolve(binding))?;
+	}
+	// we hacky using json, but we always know ABI encoding dependng on chain we
+	// run on send to
+	let cosmos_msg: CosmosMsg = serde_json_wasm::from_slice::<
+		xc_core::cosmwasm::FlatCosmosMsg<serde_cw_value::Value>,
+	>(&payload)
+	.map_err(|_| ContractError::InvalidCallPayload)?
+	.try_into()
+	.map_err(|_| ContractError::DataSerializationError)?;
+	Ok(Response::default()
+		.add_event(Event::new(XCVM_INTERPRETER_EVENT_PREFIX).add_attribute("instruction", "call"))
+		.add_submessage(SubMsg::reply_on_success(cosmos_msg, CALL_ID)))
+}
+
+/// Resolver for `BindingValue`s.
+struct BindingResolver<'a> {
+	deps: &'a Deps<'a>,
+	env: &'a Env,
+	instruction_pointer: u16,
+	tip: &'a Addr,
+	gateway: xc_core::gateway::Gateway,
+}
+
+impl<'a> BindingResolver<'a> {
+	/// Creates a new binding resolver.
+	///
+	/// Fetches gateway configuration from storage thus it may fail with storage
+	/// read error.
+	fn new(deps: &'a Deps, env: &'a Env, instruction_pointer: u16, tip: &'a Addr) -> Result<Self> {
 		let Config { gateway_address: gateway, .. } = CONFIG.load(deps.storage)?;
+		Ok(Self { deps, env, instruction_pointer, tip, gateway })
+	}
 
-		// Len here is the maximum possible length
-		let mut formatted_call =
-			vec![0; env.contract.address.as_bytes().len() * bindings.len() + payload.len()];
-
-		apply_bindings(payload, bindings, &mut formatted_call, |binding| {
-			let data = match binding {
-				BindingValue::Register(Register::Ip) =>
-					Cow::Owned(instruction_pointer.to_string().into_bytes()),
-				BindingValue::Register(Register::Tip) => Cow::Owned(tip.to_string().into_bytes()),
-				BindingValue::Register(Register::This) =>
-					Cow::Borrowed(env.contract.address.as_bytes()),
-				BindingValue::Register(Register::Result) => Cow::Owned(
-					serde_json_wasm::to_vec(&RESULT_REGISTER.load(deps.storage)?)
-						.map_err(|_| ContractError::DataSerializationError)?,
-				),
-				BindingValue::Asset(asset_id) => {
-					let reference = gateway.get_asset_by_id(deps.querier, asset_id)?;
-					match reference.local {
-						AssetReference::Cw20 { contract } =>
-							Cow::Owned(contract.into_string().into()),
-						AssetReference::Native { denom } => Cow::Owned(denom.into()),
-					}
-				},
-				BindingValue::AssetAmount(asset_id, balance) => {
-					let reference = gateway.get_asset_by_id(deps.querier, asset_id)?;
-					let amount = match reference.local {
-						AssetReference::Cw20 { contract } => apply_amount_to_cw20_balance(
-							deps,
-							&balance,
-							&contract,
-							&env.contract.address,
-						),
-						AssetReference::Native { denom } =>
-							if balance.is_unit {
-								return Err(ContractError::InvalidBindings)
-							} else {
-								let coin = deps
-									.querier
-									.query_balance(env.contract.address.clone(), denom)?;
-								balance
-									.amount
-									.apply(coin.amount.into())
-									.map_err(|_| ContractError::ArithmeticError)
-							},
-					}?;
-					Cow::Owned(amount.to_string().into_bytes())
-				},
-			};
-			Ok(data)
-		})?;
+	/// Resolves a single binding returning it’s value.
+	fn resolve(&'a self, binding: &BindingValue) -> Result<Cow<'a, [u8]>> {
+		match binding {
+			BindingValue::Register(reg) => self.resolve_register(*reg),
+			BindingValue::Asset(asset_id) => self.resolve_asset(*asset_id),
+			BindingValue::AssetAmount(asset_id, balance) =>
+				self.resolve_asset_amount(*asset_id, balance),
+		}
+	}
 
-		serde_json_wasm::from_slice(&formatted_call)
-			.map_err(|_| ContractError::InvalidCallPayload)?
-	} else {
-		serde_json_wasm::from_slice(&payload).map_err(|_| ContractError::InvalidCallPayload)?
-	};
+	fn resolve_register(&'a self, reg: Register) -> Result<Cow<'a, [u8]>> {
+		Ok(match reg {
+			Register::Ip => Cow::Owned(self.instruction_pointer.to_string().into_bytes()),
+			Register::Tip => Cow::Owned(self.tip.to_string().into_bytes()),
+			Register::This => Cow::Borrowed(self.env.contract.address.as_bytes()),
+			Register::Result => Cow::Owned(
+				serde_json_wasm::to_vec(&RESULT_REGISTER.load(self.deps.storage)?)
+					.map_err(|_| ContractError::DataSerializationError)?,
+			),
+		})
+	}
 
-	let cosmos_msg: CosmosMsg =
-		flat_cosmos_msg.try_into().map_err(|_| ContractError::DataSerializationError)?;
-	Ok(Response::default()
-		.add_event(Event::new(XCVM_INTERPRETER_EVENT_PREFIX).add_attribute("instruction", "call"))
-		.add_submessage(SubMsg::reply_on_success(cosmos_msg, CALL_ID)))
+	fn resolve_asset(&'a self, asset_id: xc_core::AssetId) -> Result<Cow<'a, [u8]>> {
+		let reference = self.gateway.get_asset_by_id(self.deps.querier, asset_id)?;
+		let value = match reference.local {
+			AssetReference::Cw20 { contract } => contract.into_string(),
+			AssetReference::Native { denom } => denom,
+		};
+		Ok(Cow::Owned(value.into()))
+	}
+
+	fn resolve_asset_amount(
+		&'a self,
+		asset_id: xc_core::AssetId,
+		balance: &Balance,
+	) -> Result<Cow<'a, [u8]>> {
+		let reference = self.gateway.get_asset_by_id(self.deps.querier, asset_id)?;
+		let amount = match reference.local {
+			AssetReference::Cw20 { contract } => apply_amount_to_cw20_balance(
+				*self.deps,
+				balance,
+				&contract,
+				&self.env.contract.address,
+			)?,
+			AssetReference::Native { denom } => {
+				if balance.is_unit {
+					return Err(ContractError::InvalidBindings)
+				}
+				let coin =
+					self.deps.querier.query_balance(self.env.contract.address.clone(), denom)?;
+				balance
+					.amount
+					.apply(coin.amount.into())
+					.map_err(|_| ContractError::ArithmeticError)?
+			},
+		};
+		Ok(Cow::Owned(amount.to_string().into_bytes()))
+	}
 }
 
 pub fn interpret_spawn(

code/xcvm/lib/core/src/instruction.rs

@@ -92,6 +92,7 @@ pub enum Instruction<Payload, Account, Assets> {
 }
 
 /// Error types for late binding operation
+#[derive(Clone, Debug, PartialEq)]
 pub enum LateBindingError<E> {
 	/// Provided late-binding is invalid
 	InvalidBinding,
@@ -104,70 +105,113 @@ pub enum LateBindingError<E> {
 /// * `payload`: Payload that is suitable for late-binding operation. Note that this API has no
 ///   assumption on the payload format or structure at all. It will only put the binding values in
 ///   the corresponding indices. If the payload were to be JSON, and `to` key supposed to have
-///   late-binding, the payload would probably look similar to this:
-/// ```{ "from": "address", "to": "" }```
-/// * `bindings`: **SORTED** and **UNIQUE** (in-terms of index) binding index-value pairs.
-/// * `formatted_payload`: Output payload. This should have enough size to contain the final data.
-/// * `binding_data`: Callback function that gives the binding data corresponding to a binding value.
-pub fn apply_bindings<'a, F, E>(
+///   late-binding, the payload would probably look similar to this: `{ "from": "address", "to": ""
+///   }`
+/// * `bindings`: **Sorted** by index binding index-value pairs.
+/// * `binding_data`: Callback function that gives the binding data corresponding to a binding
+///   value.
+pub fn apply_bindings<'a, E>(
 	payload: Vec<u8>,
-	bindings: Bindings,
-	formatted_payload: &mut Vec<u8>,
-	binding_data: F,
-) -> Result<(), LateBindingError<E>>
-where
-	F: Fn(BindingValue) -> Result<Cow<'a, [u8]>, E>,
-{
-	// Current index of the unformatted call
-	let mut original_index: usize = 0;
-	// This stores the amount of shifting we caused because of the data insertion. For example,
-	// inserting a contract address "addr1234" causes 8 chars of shift. Which means index 'X' in
-	// the unformatted call, will be equal to 'X + 8' in the output call.
-	let mut offset: usize = 0;
+	bindings: &[(u32, BindingValue)],
+	binding_data: impl Fn(&BindingValue) -> Result<Cow<'a, [u8]>, E>,
+) -> Result<Vec<u8>, LateBindingError<E>> {
+	if bindings.is_empty() {
+		return Ok(payload)
+	}
+
+	// Estimate the maximum length of the payload.  It’s ok if we don’t get this
+	// right.  If our estimate is too large we’re just waste some bytes; if it’s
+	// too small we’ll need to reallocate.  We could go through the bindings an
+	// calculate their lengths but for now we’re assuming this estimate is
+	// enough.
+	let this_len = binding_data(&BindingValue::Register(Register::This))
+		.map_err(LateBindingError::App)?
+		.len();
+	let capacity = bindings.len() * this_len + payload.len();
+	let mut output = Vec::with_capacity(capacity);
+
+	let mut start = 0;
 	for (binding_index, binding) in bindings {
-		let binding_index = binding_index as usize;
-		// Current index of the output call
-		let shifted_index = original_index + offset;
-
-		// Check for overflow
-		// * No need to check if `shifted_index` > `binding_index + offset` because `original_index
-		//   > binding_index` already guarantees that
-		// * No need to check if `shifted_index < formatted_call.len()` because initial allocation
-		//   of `formatted_call` guarantees that even the max length can fit in.
-		// * No need to check if `original_index < encoded_call.len()` because `original_index` is
-		//   already less or equals to `binding_index` and we check if `binding_index` is in-bounds.
-		if original_index > binding_index || binding_index + 1 >= payload.len() {
+		let binding_index =
+			usize::try_from(*binding_index).map_err(|_| LateBindingError::InvalidBinding)?;
+
+		if binding_index < start {
+			// The bindings weren’t ordered by index.
 			return Err(LateBindingError::InvalidBinding)
+		} else if binding_index > start {
+			// Copy literal part from the template payload.
+			let literal =
+				payload.get(start..binding_index).ok_or(LateBindingError::InvalidBinding)?;
+			output.extend_from_slice(literal);
+			start = binding_index;
 		}
 
-		// Copy everything until the index of where binding happens from original call
-		// to formatted call. Eg.
-		// Formatted call: `{ "hello": "" }`
-		// Output call supposed to be: `{ "hello": "contract_addr" }`
-		// In the first iteration, this will copy `{ "hello": "` to the formatted call.
-		// SAFETY:
-		//     - Two slices are in the same size for sure because `shifted_index` is
-		//		 `original_index + offset` and `binding_index + offset - (shifted_index)`
-		//       equals to `binding_index - original_index`.
-		//     - Index accesses should not fail because we check if all indices are inbounds and
-		//       also if `shifted` and `original` indices are greater than `binding_index`
-		formatted_payload[shifted_index..=binding_index + offset]
-			.copy_from_slice(&payload[original_index..=binding_index]);
-
+		// Resolve the binding and insert it next.
 		let data: Cow<[u8]> = binding_data(binding).map_err(LateBindingError::App)?;
+		output.extend_from_slice(&data);
+	}
+
+	// Copy remaining part of the template.
+	let literal = payload.get(start..).ok_or(LateBindingError::InvalidBinding)?;
+	output.extend_from_slice(literal);
+
+	Ok(output)
+}
+
+#[cfg(test)]
+mod tests {
+	use super::*;
 
-		formatted_payload[binding_index + offset + 1..=binding_index + offset + data.len()]
-			.copy_from_slice(&data);
-		offset += data.len();
-		original_index = binding_index + 1;
+	const FOO: BindingValue = BindingValue::Register(Register::This);
+	const BAR: BindingValue = BindingValue::Register(Register::Tip);
+	const ERR: BindingValue = BindingValue::Register(Register::Ip);
+
+	fn resolver<'a>(binding: &BindingValue) -> Result<Cow<'a, [u8]>, ()> {
+		if binding == &FOO {
+			Ok(Cow::Borrowed("foo".as_bytes()))
+		} else if binding == &BAR {
+			Ok(Cow::Owned("bar".as_bytes().to_vec()))
+		} else {
+			Err(())
+		}
+	}
+
+	fn apply(
+		template: &str,
+		bindings: &[(u32, BindingValue)],
+	) -> Result<String, LateBindingError<()>> {
+		let template = template.as_bytes().to_vec();
+		apply_bindings(template, bindings, resolver)
+			.map(|payload| String::from_utf8(payload).unwrap())
+	}
+
+	#[track_caller]
+	fn check_ok(want: &str, template: &str, bindings: &[(u32, BindingValue)]) {
+		let got = apply(template, bindings);
+		assert_eq!(Ok(want), got.as_deref())
 	}
-	// Copy the rest of the data to the output data
-	if original_index < payload.len() {
-		formatted_payload[original_index + offset..payload.len() + offset]
-			.copy_from_slice(&payload[original_index..]);
+
+	#[track_caller]
+	fn check_err(want: LateBindingError<()>, template: &str, bindings: &[(u32, BindingValue)]) {
+		assert_eq!(Err(want), apply(template, bindings))
 	}
-	// Get rid of the final 0's.
-	formatted_payload.truncate(payload.len() + offset);
 
-	Ok(())
+	#[test]
+	fn test_apply_bindings_success() {
+		check_ok("", "", &[]);
+		check_ok("foo", "", &[(0, FOO.clone())]);
+		check_ok("<foo>", "<>", &[(1, FOO.clone())]);
+		check_ok("<foobar>", "<>", &[(1, FOO.clone()), (1, BAR.clone())]);
+	}
+
+	#[test]
+	fn test_apply_bindings_failure() {
+		// Index beyond template’s length
+		check_err(LateBindingError::InvalidBinding, "", &[(1, FOO.clone())]);
+		check_err(LateBindingError::InvalidBinding, "", &[(0, FOO.clone()), (1, BAR.clone())]);
+		// Failure in resolution.
+		check_err(LateBindingError::App(()), "", &[(0, ERR.clone())]);
+		// Bindings not in sorted order.
+		check_err(LateBindingError::InvalidBinding, "<>", &[(1, FOO.clone()), (0, BAR.clone())]);
+	}
 }