Resolve identity from default config.

[fnando]

What

Resolve identity from default config.

$ stellar keys ls
alice

$ stellar keys use alice
ℹ️ The default source account is set to `alice`

$ stellar contract deploy --wasm target/wasm32-unknown-unknown/release/hello_world.wasm --alias hello --network testnet

$ stellar contract build
ℹ️ CARGO_BUILD_RUSTFLAGS=--remap-path-prefix=/Users/fnando/.cargo/registry/src/= cargo rustc --manifest-path=contracts/hello_world/Cargo.toml --crate-type=cdylib --target=wasm32-unknown-unknown --release
    Finished `release` profile [optimized] target(s) in 0.05s

$ stellar contract deploy --wasm target/wasm32-unknown-unknown/release/hello_world.wasm --alias hello --network testnet
ℹ️ Skipping install because wasm already installed
ℹ️ Using wasm hash 89123eaa2902ec32e26db0141e2d0247e5e00846489f4d8b17d597a85466e863
ℹ️ Simulating deploy transaction…
🌎 Submitting deploy transaction…
ℹ️ Transaction hash is cd91d477c058e53e1715b511071b0eb0a272890d842dab89dcf56c654fba3432
🔗 https://stellar.expert/explorer/testnet/tx/cd91d477c058e53e1715b511071b0eb0a272890d842dab89dcf56c654fba3432
ℹ️ Signing transaction: cd91d477c058e53e1715b511071b0eb0a272890d842dab89dcf56c654fba3432
🔗 https://stellar.expert/explorer/testnet/contract/CAZHGQ5I5SUDAEQRWUUWS4LHPL7K7POLD5HAOKBEZ57EAEGNYF5YZX45
✅ Deployed!
CAZHGQ5I5SUDAEQRWUUWS4LHPL7K7POLD5HAOKBEZ57EAEGNYF5YZX45

$ stellar keys address alice
GCJIGQCHDB76J5NXEZEB5JLTPV2TIS4MWKUZIEBHUSKBVLANJNSNOR2C

$ http https://horizon-testnet.stellar.org/transactions/cd91d477c058e53e1715b511071b0eb0a272890d842dab89dcf56c654fba3432 | jq .source_account
"GCJIGQCHDB76J5NXEZEB5JLTPV2TIS4MWKUZIEBHUSKBVLANJNSNOR2C"

Why

So you can actually use the value set by stellar keys use.

Known limitations

N/A

[leighmcculloch]

I think we should make the type an Option<String> instead of having an empty string value. The empty string value is not common to see in Rust used in this way, and the interface is not so nice because the --help prints out

Default value: ``

[willemneal]

+1 It was my suggestion because we were using FromStr. I think perhaps the Address enum could add a Default, case.

#[derive(Clone, Debug, Default)]
pub enum Address {
    MuxedAccount(xdr::MuxedAccount),
    AliasOrSecret(String),
    #[default]
    Config,
}

Then we can do self.source.unwrap_or_default().

[fnando]

Using Option<String> means we'll need to drop the current Address type, right (link)? This is a "raw" value that's passed to the value parser.

I'll explore Willem's suggestion with the FromStr trait.

[fnando]

Btw, without setting a default value on the arg macro, the --source-account will always be required. We can explore using default_value_t, but we'd need to return a Address-typed object. I don't think we can get away with not setting a default value with clap.

[fnando]

About the default value, clap has hide_default_value = true, which hides it completely from the --help output. Unfortunately, clap-markdown doesn't respect it (already have a ticket upstream for it).

[leighmcculloch]

Sorry, my reference to String above was a mistake. I meant Option<Address>.

[leighmcculloch]

#[derive(Clone, Debug, Default)]
pub enum Address {
    MuxedAccount(xdr::MuxedAccount),
    AliasOrSecret(String),
    #[default]
    Config,
}

I would avoid ☝🏻 because it makes the type itself recursive. Anywhere you receive the Address the consuming code would need to assume it could be Address::Config and need to recurse to get a "resolved" Address that wasn't Address::Config, but then the type still contains that variant so any consuming code would be forced to handle it.

My main issue with the approach in the PR at the moment is really not to do with the type at all, but to do with the unpacking/resolving logic happening inside the type. I think the unpacking belongs outside the type, mostly because I'd prefer types to be largely data, rather than integrated with the presentation layer. But I understand it's convenient to put it there, and sort of awkward to do it the Option

way anwyay, and we already do resolution inside from_str anyway, so LGTM. 👍🏻

[leighmcculloch]

Going to merge it as is. If we can improve it, the improvement should be a no-op non-functional refactor and we can follow up with it.

[willemneal]

The only place where this Address is pulled from the config is for the source account, so we could just handle the option is the source_account(), method. If self.source_account is None, then we use the config, otherwise it will be the Address type and it resolves as normal. I do agree that using the empty string as a special case is not very idiomatic rust and Option is the preferred way to do it. Plus my recent work adding a ContractAddress type make me realize that we need a unified approach between it and Address because in contract invoke an ScAddress could be either! So more reason not to handle the config at all in the Address type.

[leighmcculloch]

#[derive(Clone, Debug, Default)]
pub enum Address {
    MuxedAccount(xdr::MuxedAccount),
    AliasOrSecret(String),
    #[default]
    Config,
}

I would avoid ☝🏻 because it makes the type itself recursive. Anywhere you receive the Address the consuming code would need to assume it could be Address::Config and need to recurse to get a "resolved" Address that wasn't Address::Config, but then the type still contains that variant so any consuming code would be forced to handle it.

My main issue with the approach in the PR at the moment is really not to do with the type at all, but to do with the unpacking/resolving logic happening inside the type. I think the unpacking belongs outside the type, mostly because I'd prefer types to be largely data, rather than integrated with the presentation layer. But I understand it's convenient to put it there, and sort of awkward to do it the Option

way anwyay, and we already do resolution inside from_str anyway, so LGTM. 👍🏻