feat(contract invoke): support contract aliases when parsing address #1765
Conversation
willemneal
force-pushed
the
feat/contract_aliases_in_sc_addr_parsing
branch
from
d391741 to
e739a46
Compare
willemneal
force-pushed
the
feat/contract_aliases_in_sc_addr_parsing
branch
from
7d710e8 to
343eeb1
Compare
| let contract = UnresolvedContract::resolve_alias(&alias, locator, network_passphrase); | ||
| let muxed_account = | ||
| super::UnresolvedMuxedAccount::resolve_muxed_account_with_alias(&alias, locator, None); | ||
| match (contract, muxed_account) { | ||
| (Ok(contract), _) => Ok(xdr::ScAddress::Contract(xdr::Hash(contract.0))), | ||
| (_, Ok(muxed_account)) => Ok(xdr::ScAddress::Account(muxed_account.account_id())), |
There was a problem hiding this comment.
Just realized that if the alias for a contract and key are the same, we choose the contract over the key. Is this the correct behavior? Should we return an error? Or do we want to ban the same names across the two namespaces when adding?
There was a problem hiding this comment.
I don't think we should ban the same names (logically they belong to different namespaces), maybe we can add a context to this function? resolve_key()/resolve_contract(). The caller always know if it wants a contract or a key to be returned.
There was a problem hiding this comment.
NVM I misunderstood the problem.
Maybe we should indeed error in this case and make user to specify the context directly.
E.g. if I have aliases main for both keys and contract I can pass it as main@key or main@contract and we can mention this in error message
$ contract invoke my-contract -- --key main@key
# ok
$ contract invoke my-contract -- --key main@contract
# ok
$ contract invoke my-contract -- --key main
# error: duplicate aliases found for both key and contract, please specify which alias should be used: either main@key or main@contractThere was a problem hiding this comment.
logically they belong to different namespaces
Actually, I think we should think about them as one namespace, and the issue is they shouldn't overlap.
It's necessary to think of them as one namespace because there is a single Soroban type that represents them both, an Address, and both can always be passed in, so therefore the alias must be unique across both.
So I think we should prevent using contract names that overlap with key names, and vice-versa.
There was a problem hiding this comment.
Or do we want to ban the same names across the two namespaces when adding?
☝🏻 We should do this.
But, given that aliases may already exist that clash, then I think we should also return an error if an alias that refers to either is used in a location that could be either. The error can output the addresses for both, so that people who get into this situation can just copy-paste the G or C address into where they are trying to use the alias.
There was a problem hiding this comment.
It's necessary to think of them as one namespace because there is a single Soroban type that represents them both, an Address, and both can always be passed in, so therefore the alias must be unique across both.
Hm but don't they belong to 2 different logical entities? (address (G..) vs contract(C..))
Not sure if users think of them as the same entity
I do agree it's easier to restrict users to not be able to have duplicate aliases
There was a problem hiding this comment.
They are one logical entity in contracts. Contracts see an "Address" type, and the contents are opaque. While they're separate concepts in the CLI, I don't think we win enough by introducing a new format for people to learn like myalias@key/contract.
fixes: #1764