Fix Design of Account

[jonjove]

What problem does your feature solve?

The design feels strange. Specifically:

• from_public_key uses low_threshold to check if the account exists immediately. It should be possible to avoid this check (it isn't free). Notably, it means that you shouldn't construct an Account in a place where you aren't sure you are about to use it because it will impact the footprint for no reason.
• from_public_key constructs from FixedBinary instead of Binary. It should work on Binary because the guest shouldn't be forced to do a length check that can be done in the host. Binary -> FixedBinary is basically free, the opposite requires at least one host function call.

What would you like to see?

Fixes to the above.

What alternatives are there?

None

[jonjove]

I wrote that backwards. It should say FixedBinary -> Binary is basically free, the opposite requires at least one host function call.

[leighmcculloch]

When I made this change from what I could tell the example contracts all use FixedBinary<32> as the public key type, which probably seems appropriate? But it can be changed to Binary, that doesn't seem like a big issue. Suggest we plan to do that in next iteration, or reevaluate then.

Regarding the error case, I don't think we should allow the threshold and signer fns to be called if they trap on non-existent account, and I don't think we should return Result's from all of them. If someone doesn't want to check if an account exists at a point in time, they probably shouldn't use the Account type until they need an account that exists?

• from_public_key uses low_threshold to check if the account exists immediately.

This is temporary because the host fns do not provide a way to check if the account exists or not. Once stellar/rs-soroban-env#262 is done we can make the Account::from_public_key return a Result properly.

https://github.com/stellar/rs-stellar-contract-sdk/blob/0f48b9caacd08a2531351de2cf2552faabaaccb8/sdk/src/account.rs#L171-L174

Does that address the issue you're seeing?

[leighmcculloch]

We could probably do with another type I think defined in the SDK: type AccountId = FixedBinary<32>, and encourage use of AccountId anywhere an account identifier is needed, without an actual account needing to exist. Then we can also provide a .get_account(&self) function for AccountId too.

An example of this is in #353.

[jonjove]

Let's handle this in the next iteration. I don't think I totally agree with you regarding the error case, but let's discuss when we're ready to do something.

[jonjove]

One specific case where you might want to construct something that you aren't sure to use is immediately before a loop. The loop can have any number of iterations, and you don't want to reconstruct it every iteration (especially if constructing it automatically incurs the cost of an extra check). But if the loop has 0 iterations then it was a waste.

[leighmcculloch]

I think that optimization might be lost in the noise, but I think you could break out the first iteration of the loop to do expensive setup. It's worth noting that @graydon has discouraged us from optimizing away host fn calls due to their cheapness. If anything, adding branches to avoid host fn calls will probably have a greater expense? We should experiment and measure this, as with many other optimizations we probably need to make.

[leighmcculloch]

Another option which I think is also viable is to add a does_exist fn to Account, and it is the users responsibility to call it when they need to before calling other fns, but we're not leaning on the Rust to help developers write secure code.

[jonjove]

When I made this change from what I could tell the example contracts all use FixedBinary<32> as the public key type, which probably seems appropriate? But it can be changed to Binary, that doesn't seem like a big issue. Suggest we plan to do that in next iteration, or reevaluate then.

This is true right now. But it isn't necessarily true for data in storage. For example, the liquidity pool example stores the contract ID as a Binary to avoid checking its length every time--it knows the length because it stored it.

[leighmcculloch]

Let's see if we can omit the length check for direct RawVal to FixedBinary conversions, since this is likely to be an optimization that would be valuable in many places, otherwise the FixedBinary type is not particularly useful if it is to be avoided.

[jonjove]

I don't like that idea. FixedBinary should always check that it meets the requirement of the type. It's still useful for user input (which is how the contract examples use it) because it guarantees the checks.

But let's drop this discussion until the next iteration.

[leighmcculloch]

I don't like that idea. FixedBinary should always check that it meets the requirement of the type.

I think I was misunderstood. I'm saying the check remains, however for RawVal => FixedBinary conversions the check would occur host side. If you convert from Binary to FixedBinary locally then yes the check would remain, but if an application plans to accept and use FixedBinary end-to-end for account IDs, any of those checks should ideally be host side rather than repeated locally. Assuming this is an optimization worth doing.

[leighmcculloch]

Related to #369