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

Add support for contract id alias name when deploying and invoking contracts. #1356

Merged
merged 26 commits into from
Jun 11, 2024

Conversation

fnando
Copy link
Member

@fnando fnando commented Jun 4, 2024

What

soroban contract deploy

This pr introduces two different switches:

  • --alias ALIAS: the alias name, effectively the file name that will be created under .soroban/contract-ids/:network_name/:alias.txt. If --global is provided, then the alias will be created under ~/config/.soroban/contract-ids/:network_name/:alias.txt.

soroban contract invoke

When invoking a contract, if the user provides a valid alias to --id, then the respective contract id will be loaded. If the alias doesn't exist, then an error message like error: cannot parse contract ID foo: the strkey is invalid will be returned.

Why

When deploying wasms, it's common to redirect the contract id to a file so it can be reused later on. To simplify the whole process and make this a non-*nix feature, let's introduce the concept of contract id aliasing.

Known limitations

  • This pr optimistically tries to create and read the contract id file.

Tickets

@fnando fnando changed the title Optimistically save contract id alias name to local soroban directory. Optimistically save contract id alias to soroban's config directory. Jun 4, 2024
@willemneal
Copy link
Member

Overall a great first big rust PR. As mentioned above I do think that these functions would work best in the config so that we can separate them from the commands.

And I mentioned yesterday that I think that aliases should be tied to the network where the contract was deployed to. Perhaps for now we can use local, mainnet, testnet, and futurenet as subdirectories. I know you had another issue to stream line adding networks using these.

@fnando
Copy link
Member Author

fnando commented Jun 5, 2024

@willemneal can you please do a second review pass? Will extract everything to config::Args in a separate pr, because I noticed the change surface it's way too big.

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 had the thought, that install and deploy are separate, and maybe there's also value in aliases for contracts installed, and not just aliases for deployments of installed contracts. I don't think it changes anything significant about what we're doing here, but just keeping that in mind that we may also end up with aliases pointing to different things. In this case they could still be separate ID spaces so I think what we're doing here, making aliases specifically for contract IDs makes sense.

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.

I think there's a bug, that doesn't actually result in negative outcome for the user as it is invisible really, but the behaviour doesn't seem correct. See below.

cmd/soroban-cli/src/commands/contract/deploy/wasm.rs Outdated Show resolved Hide resolved
@fnando fnando changed the title Optimistically save contract id alias to soroban's config directory. Add support for contract id's alias name when deploying and invoking contracts. Jun 6, 2024
@fnando fnando changed the title Add support for contract id's alias name when deploying and invoking contracts. Add support for contract id alias name when deploying and invoking contracts. Jun 6, 2024
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.

Looks good. I would make the changed noted below if you're up for it. It is a more common idiom in Rust than using empty strings to signal an absent value. I hadn't mentioned it before, but mentioning it now because I think that was the root cause of the bug that was fixed.

cmd/soroban-cli/src/commands/contract/deploy/wasm.rs Outdated Show resolved Hide resolved
@leighmcculloch
Copy link
Member

The RPC tests look like they're failing for reasons that aren't obvious. They aren't failing on other PRs that are open, so it might be something to do with this change.

@fnando
Copy link
Member Author

fnando commented Jun 10, 2024

@leighmcculloch @willemneal I've changed the alias file to be scoped by network passphrases. This means we'll have only one file for everything.

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

$ cat ~/.config/soroban/contract-ids/alice.json | jq
{
  "ids": {
    "Test SDF Network ; September 2015": "CDMXLF7V4XQ4QZ62D6MT2J2SGY4YLBHPOQTT3WPXRHTGQ4CUFUE3JW5E",
    "Standalone Network ; February 2017": "CCOZHG2BHABPGVIYBJP675PHDFCT5LPVVY5GAX5CGOWI274DAN6PPUZ5"
  }
}

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

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

Can you please review the latest changes and suggest only extremely required changes? I'd like to merge this in so I can fix the contract init's generated file before the next CLI release (today-tomorrow). We can iterate and apply other changes in further pull requests. :)

@leighmcculloch leighmcculloch enabled auto-merge (squash) June 11, 2024 01:46
@leighmcculloch leighmcculloch merged commit e189c71 into main Jun 11, 2024
25 checks passed
@leighmcculloch leighmcculloch deleted the alias branch June 11, 2024 01:47
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