Add new error code CHANGE_TRUST_NO_TRUST_LINE to "change trust" operation

[ire-and-curses]

If I use the "change trust" operation to try and delete a trustline that isn't there, I get CHANGE_TRUST_INVALID_LIMIT. According to the documentation, the description of this error is

The limit is not sufficient to hold the current balance of the trustline and still satisfy its buying liabilities.

This may technically be correct (if the referenced trustline doesn’t exist, then change trust tries to create a new one but it is invalid to create a trustline with limit 0 since that would delete it), but this is not a helpful or obvious error.

It would be better if "change trust" returned a new error code, CHANGE_TRUST_NO_TRUST_LINE, analogous to ALLOW_TRUST_NO_TRUST_LINE, ACCOUNT_MERGE_NO_ACCOUNT, MANAGE_DATA_NAME_NO_FOUND, PAYMENT_NO_ISSUER etc. Then the caller would have a clear explanation for what they did wrong.

[theaeolianmachine]

@ire-and-curses if you want to take a crack at a CAP, I don't think this would get a lot of debate, and the XDR change should be minimal.

[ire-and-curses]

Ok, email sent to stellar-dev.

[nebolsin]

@ire-and-curses I agree that the current behaviour could be misleading, and a dedicated error code for this case would definitely be an improvement. But should change_trust(0) on non-existent trustline be an error in the first place?

The semantics of change_trust operation with non-zero limit suggests that this operation is declarative, meaning that we only describe the desired state of the system, and stellar-core takes an appropriate action (create or update) to transition to this state. If the system ends up in the requested state — it's a success; if it cannot reach that state — it's an error. If the system is already in the requested state — no changes are necessary and the operation succeeds.

Following this logic, change_trust(account, asset, 0) means that we want to transition to a state in which account has no trustline for asset. And if the system is already in the desired state — it seems logical to happily report a success.

Here's an example to illustrate the inconsistency of current change_trust behavior:

1. Create and fund a new account
2. Submit a tx with 2 identical change_trust(account, asset, 1000) operation for some existing asset — both would succeed.
3. Submit a similar tx with two change_trust(account, asset, 0) operations — first succeeds, second fails with op_invalid_limit.

To summarize, the alternative proposal is to make change_trust(account, asset, 0) result with success when account has no trustline for asset. What do you think?

[theaeolianmachine]

Hmmmm, that is interesting. @jonjove do you have thoughts on this?

Typically the values that the core team has followed when it comes to errors is that it's better to fail fast and bubble errors so that SDKs and higher level systems are aware of things, so normally I'd advocate for an error. However, I completely agree with @nebolsin that it definitely adds additional logic for implementers and isn't easy to understand, and think it might make more sense to not throw an error (because as mentioned, it really didn't change anything if it didn't already exist).

[MonsieurNicolas]

Of note, it's a similar problem than for order book changes: it's "easy" to replace a success code by a different success code (or failure code by a different failure code), it's hard to change a failure to a success (or opposite) as it potentially breaks smart contracts

[nebolsin]

@MonsieurNicolas That's certainly a good general rule, but I cannot think of any smart contract which can significantly depend on a fact that the attempt to delete a non-existent trustline result in a failure. Can you share any possible use cases?

To me, it seems that this non-idempotency of change_trust(0) can only be bad for smart contracts, because it makes the operation more brittle and sensitive to the initial state. And it also seems inconsistent with the behavior of change_trust(max_limit), which would succeed regardless of the trustline existence.

That's probably not a big deal, and it's possible to use the protocol as it is now, even without the new error code. Just thought that it might be a relatively harmless change which will simplify operation semantics and improve the developer experience. And I guess it will only become harder to make such changes as the network utilization grows.

[theaeolianmachine]

Yeah, I completely agree — I don't believe that smart contracts today should be the stopgap for fixing this now as opposed to later, and think that's well worthwhile putting into a CAP.