Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extract alias logic into its own implementation. #1369

Merged
merged 9 commits into from
Jun 14, 2024
Merged

Conversation

fnando
Copy link
Member

@fnando fnando commented Jun 11, 2024

What

Extract alias logic into its own implementation.

Why

So we can easily reuse it in other commands (e.g. contract bindings typescript will also load the contract id alias).

Known limitations

N/A

Storing alias locally

$ soroban contract deploy --wasm ./target/wasm32-unknown-unknown/release/hello_world.wasm --network standalone --source alice --alias alice
CDMCQHHE356XOGBXXUTZ3AVVARRTRYKHFVYAC6UYWM7RYPKSYV2FUPDS

$ cat .soroban/contract-ids/alice.json | jq
{
  "ids": {
    "Standalone Network ; February 2017": "CDMCQHHE356XOGBXXUTZ3AVVARRTRYKHFVYAC6UYWM7RYPKSYV2FUPDS"
  }
}

$ soroban contract invoke --network standalone --source alice --id alice -- hello --to fnando
["Hello","fnando"]

Storing alias globally

$ soroban contract deploy --wasm ./target/wasm32-unknown-unknown/release/hello_world.wasm --network standalone --source alice --alias alice --global
CDNSYIZ7IUPFGP72DATK4DYOU5ZKPUBBHBTAIFUBTCEDK3RR5WRQOP7F

$ cat ~/.config/soroban/contract-ids/alice.json | jq
{
  "ids": {
    "Standalone Network ; February 2017": "CDNSYIZ7IUPFGP72DATK4DYOU5ZKPUBBHBTAIFUBTCEDK3RR5WRQOP7F"
  }
}

$ soroban contract invoke --network standalone --source alice --id alice --global -- hello --to fnando
["Hello","fnando"]

@willemneal
Copy link
Member

Looks good so far. One suggestion is to move to it's own file under config. Then you can name the types Error and Data. Then externally, it would be alias::Error and alias::Data.

@willemneal
Copy link
Member

Looking good! Just one more suggestion of moving the logic to config this way we later if it becomes it's own crate we can use it to load_contract_ids from aliases to import them in smart contracts.

@willemneal
Copy link
Member

@leighmcculloch @fnando One more thought designwise, is it possibly confusing to users that --contract-id could be an --contract-alias and move this into its own Arg? This commands that need a contract_id

 Cmd {
    #[flatten]
    contract_id: contract_id::Arg,
    /// ...
    }

and it could be

struct Arg {
    #[arg(long = "contract-id", required_unless_present = "contract-alias")]
    pub contract_id: Option<String>,
    #[arg(long = "contract-alias", conflicts_with = "contract-id")]
    pub contract_alias: Option<String>,
}

impl Arg {
    pub fn resolve(&self, config: &config::Args) -> Result<Strkey::Contract, Error>
}

Then for all commands that use the contract id it would be self.contract_id.resolve(&config)?.

I'm totally fine either way just always am a fan of putting shared logic into flattened Args and not overloading args.

@fnando
Copy link
Member Author

fnando commented Jun 13, 2024

@willemneal as for --contract-alias, I think we shouldn't add it, just because is yet another thing to keep in mind when using the CLI. But one I think we should do is improving the documentation for --contract-id mentioning the support for aliasing.

As soon as the alias work is done, I plan to update the docs with a section for contract id aliasing, which should also help with adoption.

Copy link
Member

@leighmcculloch leighmcculloch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one piece of feedback inline, but regardless looks good to merge.

cmd/soroban-cli/src/commands/config/alias.rs Outdated Show resolved Hide resolved
cmd/soroban-cli/src/commands/config/alias.rs Show resolved Hide resolved
Copy link
Member

@willemneal willemneal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fnando Looking really great! Much cleaner for callers to use it. Just one nit.

cmd/soroban-cli/src/commands/config/alias.rs Outdated Show resolved Hide resolved
@fnando
Copy link
Member Author

fnando commented Jun 14, 2024

@willemneal can i get a 👍 ?

Copy link
Member

@willemneal willemneal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still think that load is too general, but it's now no longer public so we can address in a future PR. Good work!

@willemneal willemneal merged commit 52dce61 into main Jun 14, 2024
25 checks passed
@willemneal willemneal deleted the alias-extract branch June 14, 2024 20:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants