Entered unreachable code at `ExecutionFinalResult::into_result`

[Sliman4]

near-api-rs/types/src/transaction/result.rs

Lines 231 to 248 in db278e4

	impl ExecutionFinalResult {
	/// Converts this object into a [`Result`] holding either [`ExecutionSuccess`] or [`ExecutionFailure`].
	#[allow(clippy::result_large_err)]
	pub fn into_result(self) -> Result<ExecutionSuccess, ExecutionFailure> {
	match self.status {
	FinalExecutionStatus::SuccessValue(value) => Ok(ExecutionResult {
	total_gas_burnt: self.total_gas_burnt,
	value: Value::from_string(value),
	details: self.details,
	}),
	FinalExecutionStatus::Failure(tx_error) => Err(ExecutionResult {
	total_gas_burnt: self.total_gas_burnt,
	value: tx_error,
	details: self.details,
	}),
	_ => unreachable!(),
	}
	}

I'm running v0.8.0 of this library which doesn't seem to have changed since that version. Possibly caused by .wait_until(TxExecutionStatus::Included)

I understand that there's nothing useful to return before execution, but it's better to return nothing (Option maybe) than unexpected undocumented panics. Maybe it's too early to return execution final result if there's nothing final or executed yet.

Unrelated but I would actually prefer being able to use real types as in previous version of this crate instead of calling into_result on everything that implies unnecessary cloning. Or extending FinalExecutionStatus (and similar enums) to behave like Result (is_err, is_success, unwrap, unwrap_err, inspect, inspect_err, map, map_err, ok, ok_or, etc. and of course you can always match it). I found a lot of cases while migrating to a new version of near-api where what I used to do in 1-2 lines of code I had to write 10 since the fields are now pub(crate)

[akorchyn]

Hey, from which version have you migrated? If you are migrating from 0.6, then you probably also are migrating to open-api types, and they are not that convenient to use directly compared to nearcore ones.

That's why we added an extra wrapper. It was also easy to miss the processing execution result before. And you might expect that the transaction succeeded, when in fact the transaction failed.

However, I agree that we should iterate over the result type a bit more, as currently, it's moved from near-workspaces, and I didn't spend much time editing it.

[Sliman4]

Yeah, the result type also felt strange to use in workspaces, but manageable because in tests you can just unwrap everything until you get what you want, and the more panics the better. In production code you don't really do that.

It was also easy to miss the processing execution result before. And you might expect that the transaction succeeded, when in fact the transaction failed.
I believe there was #[must_use] on it, wasn't there? Either way, must_use is the way, rust Result fully relies on it, as well as Future that was not awaited