-
Notifications
You must be signed in to change notification settings - Fork 70
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
Deploy contract to RPC server #152
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this is a WIP, but I'm mentioning some things inline in case they help.
src/deploy.rs
Outdated
let separator = | ||
b"create_contract_from_ed25519(contract: Vec<u8>, salt: u256, key: u256, sig: Vec<u8>)"; | ||
let mut hasher = Sha256::new(); | ||
hasher.update(separator); | ||
hasher.update(salt); | ||
hasher.update(contract); | ||
let hash = hasher.finalize(); | ||
|
||
let sig = key.sign(&hash); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once stellar/rs-soroban-env#473 is merged (should be soon), which adds create_contract_from_source_account
, we should remove these lines of code and do the deploy from the source account of the transaction. You won't need to sign the internals, just the outside Stellar tx which is simpler and we only need to handle the simple case from the CLI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is create_contract_from_source_account
documented?
What's the advantage of using it?
And finally, will the current approach still work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is
create_contract_from_source_account
documented?
All host functions are captured in this file:
https://github.com/stellar/rs-soroban-env/blob/e110dff77241cee23f6b274022b38bf4210f8577/soroban-env-common/src/env.rs#L308
The documentation is sparse on it, so adding some in stellar/rs-soroban-env#488.
What's the advantage of using it?
It requires less signatures as no auth on the actual invocation is required. The Stellar transaction inherently provides the auth. This is also the only way a wallet like Freighter will support signing starting out, so the soroban-cli serve command needs to support deployment via this way, so may as well make it the only way for now to deploy in the CLI.
And finally, will the current approach still work?
Maybe, can you see the link to the issue here: https://stellarfoundation.slack.com/archives/C030Z9EHVQE/p1664297563128759
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@leighmcculloch , which cap/doc has writeup on how the InvokeHostFunction operation/tx is assembled per shown here in rs? I'm transposing this to horizon/go, and wanted to use it as reference if available, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're looking for CAP-47, but it hasn't been updated to include create_contract_from_source_account
yet.
4e9b15b
to
ca9e7ac
Compare
@paulbellamy @leighmcculloch PTAL, I still need to migrate this to use |
7c580bb
to
c784cd2
Compare
src/deploy.rs
Outdated
.request("sendTransaction", rpc_params![tx.to_xdr_base64()?]) | ||
.await?; | ||
println!("{}", response.status); | ||
Ok(()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should probably poll getTransactionStatus
to wait for the "status": "success"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, so sendTransaction
isn't synchronous?
What's the meaning/point of the status field in the sendTransaction
response then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@paulbellamy I implemented it, PTAL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing I think we need to change regarding the private key, which is small tweak.
src/deploy.rs
Outdated
// TODO: we should probably use an env variable for security reasons | ||
// (otherwise it will recorded in the shell history) | ||
/// Private key to sign the transaction sent to the rpc server | ||
#[clap(long = "private-strkey")] | ||
private_strkey: Option<String>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to not require private keys to be provided via command line arguments, as it leaks the private keys into computer logs, process lists, etc. It should be collected either via a file (not fun) or via a prompt. In our other CLI tools that accept private keys (such as stc
) we avoid collecting secrets via arguments for this reason.
Can you enable the "env"
feature of the clap
lib, and add a env
argument to the clap
attribute? That will make this field settable via an env var.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's why I wrote the TODO. I will do it in a later PR if you don't mind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is to be merged without this, can you make sure it happens before the tool is released?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, will do
What
Allow deploying contract to remote RPC server instead of local sandbox
Why
#41
Known limitations
WIP