#Address::account_id 🧐

45 messages · Page 1 of 1 (latest)

hallow saffron
#

The SDK exposes a function on Address called account_id and it exposes a BytesN<32> which is the ed25519 public key.

This is the first place as far as I can tell in the protocol where we break out of otherwise consistent support for future account id key types. Up until now account IDs everywhere in the protocol support being extended with other types.

It's not really clear to me why a developer would need to use this function. What do we hope or expect they'll do with it?

#

We also have this unfortunate situation where we technically could have a contract ID and an account ID use the same 32 byte value. So for contracts we have a bit of an identifier overlap problem if someone is naive to use it and mix it with contract IDs as well.

#

I guess the possibility of this happening is so small and impossible.

#

Because of how contract IDs are generated.

silent snow
#

I just wanted back-and-forth conversions for the addresses; this one is probably not as useful indeed

hallow saffron
#

I get that. From a 'provide all the primitives' point-of-view, it makes sense for us to provide a way for folks to get at these values.

silent snow
#

we break out of otherwise consistent support for future account id key types.
I don't think so - we could have something like Address::account_id_64 if needed

#

(need to check discord history though; there definitely were feature requests about address conversions in general)

hallow saffron
#

I saw quite a few address conversions in initial contracts people wrote, and in every case I engaged it turned out it was unnecessary, and folks were just drawn to that idea of "gotta get the account to do stuff with it".

#

This was back when we had Invoker Auth, and the Address type was a Rust enum and so it was a little easier to get it out. I like that we've made it more opaque.

silent snow
#

yeah, there were no direct account support requests

hallow saffron
#

We should probably make this Address::account_id_ed25519_public_key. What's being returned isn't the account ID, but the ed25519 public key for the account ID. And the only reason really to get at it is to do ed25519 key verification, which imo is ⚠️ dangerous ⚠️ because Account ID public keys on Stellar should never be blindly used for verification, because of Stellar's multisig setup. So actually, I'm thinking it might worth hiding this.

silent snow
#

so we could drop this if needed

#

or rename, yeah

#

probably removing is a safer option

hallow saffron
#

The reason it is dangerous is because a developer can never assume a ed25519 public key for an account provides any authorization for the account, because the master key might have been compromised and removed as a signer. So this is really only useful if we expose a signers interface like what we had before, and I agree with your previous reasoning for why we don't want that anymore.

silent snow
#

the only valid scenario I can think of now is hardcoding an account address

#

(that's definitely avoidable, but it may come up)

hallow saffron
#

🤔 That's a good point. People can still do that in tests by do a conversion from xdr::AccountId to sdk::Address, which is supported. Outside of tests should people hardcode Ids? Probably not, but it is convenient.

silent snow
hallow saffron
#

Thanks.

silent snow
#

ideally we'd have some 'instance storage' to avoid hardcoding while providing the same cost-efficiency, but it doesn't seem like we have enough time for that now

hallow saffron
#

There are two issues I see in that thread:

#
  1. The dual BytesN<32> and Address types being used for contract IDs creating problems for folks.
#
  1. The situation where someone tries to do something with an account ID Address that is only valid with a contract ID Address.
#

We have a story for (1): getting rid of BytesN<32> for contract IDs in the SDK API.

#

For (2): I get it, but think it is a non-issue. Someone trying to call a contract function on an Account ID is no different to someone trying to call a contract function on a Contract ID that doesn't exist. It just doesn't exist as a Contract to call on.

#

(cc @daring apex who raised that issue)

silent snow
#

yeah, that sounds fair to me

hallow saffron
#

The request in that thread to:

if Address becomes the default, it would be nice to be able to store an Address in-code as a constant

I get it, but does it scale? Addresses are network dependent. Contracts can have an initialize function that accept an Address. Seems like a simple work around.

hallow saffron
#

Although, it's achievable today by writing a contract function to set the data too 🤔

silent snow
#

hmm, yeah, we could create an issue for that. FWIW the we came back to this quite a few times in different contexts (e.g. in rent context)

hallow saffron
#

What's the relationship to rent?

silent snow
#

I'm not sure it's relevant anymore, but the idea was about the fact that contract instances are kind of special in a sense that it seems fair to bump rent for them automatically. but the 'instance state' itself is just a regular ledger storage without any clear properties (so it's not clear if autobumps are fair/needed)

#

that's not a huge deal though. we could open an issue and revisit it as needed

hallow saffron
#

I'm going to open a proposal PR to remove account_id, and then after adopting Address, also propose removing getting the contract ID out too.

daring apex
hallow saffron
#

I also think if we're going to start needing a in-bytes way to refer to these things, we need to switch to strkeys. Strkeys are there to provide a canonical single common representation of all of our IDs. They're the thing we have that preserve context.

#

Our tooling like the CLI is already, afaik, switching to using C strkeys for contract IDs. We shouldn't need bytes versions anymore soon.

#

We might need to add host functions for conveniently converting from a strkey to an Address.

daring apex
#

One other minor use case that would no longer be possible is being able to hash the account_id. For example, a factory contract that creates the salt off of the account and some other data. But this is quite minor and should be easy to work around.

hallow saffron
hallow saffron
hallow saffron
#

@silent snow Do you think it's reasonable for a contract developer to want to check if an Address is a contract? The only case I can think of is if someone wants to check if it's something they can call, but there's no guarantee a call will succeed in any case, so I think the right thing to do is to use a try_ call in that case.