solana: Fix logic error in Inbox release process

[johnsaigle]

There are a few suspicious aspects of the inbox release logic:

1. release_inbound_unlock takes a flag that determines whether a false result from try_release should result in an error being returned. The flag indicates that it should revert on delay. However, try_release can also return false when a transaction has not been approved (regardless of whether or not it is delayed).
   If the flag is set, the transaction will not return an error when a user tries to release a transaction that is not approved even though whether a transaction is approved has nothing to do with its expected behaviour when it is delayed.

Context:

try_release() returns Ok(false) when a transfer is not approved (L37)

example-native-token-transfers/solana/programs/example-native-token-transfers/src/queue/inbox.rs

Lines 33 to 47 in 93350ab

	pub fn try_release(&mut self) -> Result<bool> {
	let now = current_timestamp();
	match self.release_status {
	ReleaseStatus::NotApproved => Ok(false),
	ReleaseStatus::ReleaseAfter(release_timestamp) => {
	if release_timestamp > now {
	return Ok(false);
	}
	self.release_status = ReleaseStatus::Released;
	Ok(true)
	}
	ReleaseStatus::Released => Err(NTTError::TransferAlreadyRedeemed.into()),
	}
	}

release_inbound_mint (and release_inbound_unlock) return Ok(()) when transfer not approved and the revert_on_delay flag is false (L75)

example-native-token-transfers/solana/programs/example-native-token-transfers/src/instructions/release_inbound.rs

Lines 63 to 79 in 93350ab

	pub fn release_inbound_mint(
	ctx: Context<ReleaseInboundMint>,
	args: ReleaseInboundArgs,
	) -> Result<()> {
	let inbox_item = &mut ctx.accounts.common.inbox_item;
	let released = inbox_item.try_release()?;
	if !released {
	if args.revert_on_delay {
	return Err(NTTError::CantReleaseYet.into());
	} else {
	return Ok(());
	}
	}
	assert!(inbox_item.release_status == ReleaseStatus::Released);

2. The custom error TransferNotApproved is defined but unused in the codebase

Taken together, I believe try_release() should revert with the TransferNotApproved error. This would disambiguate the various 'false' results and provide more intuitive behaviour when a user tries to release a transaction that is not approved.

Current behaviour:

• if revert_on_delay == false and ReleaseStatus::NotApproved, try_release() returns Ok(())
• if revert_on_delay == true and ReleaseStatus::NotApproved, try_release() reverts with CantReleaseYet

New behaviour:

• if revert_on_delay == false and ReleaseStatus::NotApproved, try_release() reverts with TransferNotApproved
• if revert_on_delay == true and ReleaseStatus::NotApproved, try_release() reverts with TransferNotApproved

Note that funds are safe in both cases. assert statements in both release_inbound functions check that the ReleaseStatus is Redeemed before minting funds.

[johnsaigle]

The actual argument is called revert_on_delay. But perhaps this flag should revert on any error and not just the case where the transaction is not ready to be released?

[kcsongor]

I think we should keep the existing behaviour, but potentially think of a name for the flag that better describes it? Something like revert_when_not_ready...

The thinking here is that it's convenient for the client to send all the instructions in a single transaction (the transceiver redeem one, then the inbound release one). Whether the transaction can be redeemed immediately cannot be determined on the client side atomically, so the client would either have to perform two transactions, or do some non-trivial computation (involving a lookup) to figure out whether the transceiver redeem ix hits the threshold.

The intention of this flag is definitely to "attempt to redeem if we can". I originally called the flag "revert on error" (as you point out in the comment), but that's not a very good name because the function errors out if the message is already release regardless of the flag.

[johnsaigle]

OK, makes sense. I'm happy with renaming the flag.
The last thing I'm wondering about though is this case:

if revert_on_delay == true and ReleaseStatus::NotApproved, try_release() reverts with CantReleaseYet

Maybe this could return TransferNotApproved? It might be helpful for a user to know why their transfer can't be released given that there are two different reasons why. It gives them a way to solve the problem

[kcsongor]

Yeah that makes sense to me. Maybe the cleanest way to do that is to propagate the boolean flag to try_release and have it revert with the appropriate message.

[johnsaigle]

Sounds good to me. I'll update this PR

[kcsongor]

not sure it's worth introducing a breaking change at this point