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

feat: update to newest soroban-rpc and copy over old signing logic #1362

Merged
merged 11 commits into from
Jun 14, 2024

Conversation

willemneal
Copy link
Member

@willemneal willemneal commented Jun 6, 2024

What

Add a Stellar trait for signing (up for suggestions for a better name Signer seemed overloaded). This will allow for other implementations like the Ledger in a future PR. An initial implementation of the trait LocalKey uses a local signing key and has the ability to prompt the user to confirm the signature. Currently this defaults to false to match the current behavior.

Also updated to use the newest soroban-rpc and used the new signer for all relevant commands. And copied over the signing code from the RPC to make this PR more minimal.

Why

The soroban-rpc was updated to remove all logic that didn't pertain to core RPC methods.

Known limitations

[TODO or N/A]

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 left some comments inline. Mostly minor. A couple areas of commented code and it's unclear if the code is incomplete or what the commented code is for.

There's one area that reads like a bug to me but I might be missing something. (#1362 (comment))

Can we remove the prompting code from this change since it's dead code? Or comment it. It includes a dependency that won't be used.

This change contains more new code than I think we anticipated this late in the release process that touches a high sensitive area. But I agree it looks like it doesn't change the experience because the new functionality is disabled, but I'm just guessing because the code isn't identical, and I can't tell for sure, especially given the possible bug.

I think this PR is heading in the right direction for signing, and is what we'd want to head towards.

My preference is that we verbatim copy-paste the code that was deleted from stellar/stellar-rpc@587a229 and paste it into the stellar-cli, and use that for the final v21 release, then move forward with merging this PR for v22.

(The prompting is so helpful from a security stand point we might actually want to release this in a v21 minor version as a breaking change, so I don't think we'd be delaying merging this till v22.)

cc @fnando @chadoh ☝🏻 if you have alternate opinions.

cmd/soroban-cli/src/commands/contract/deploy/wasm.rs Outdated Show resolved Hide resolved
cmd/soroban-cli/src/commands/contract/invoke.rs Outdated Show resolved Hide resolved
cmd/soroban-cli/src/commands/contract/invoke.rs Outdated Show resolved Hide resolved
cmd/soroban-cli/src/signer.rs Outdated Show resolved Hide resolved
cmd/soroban-cli/src/signer.rs Outdated Show resolved Hide resolved
cmd/soroban-cli/src/signer.rs Outdated Show resolved Hide resolved
cmd/soroban-cli/src/signer.rs Outdated Show resolved Hide resolved
cmd/soroban-cli/src/signer.rs Outdated Show resolved Hide resolved
cmd/soroban-cli/src/signer.rs Show resolved Hide resolved
cmd/soroban-cli/src/commands/config/mod.rs Outdated Show resolved Hide resolved
@willemneal
Copy link
Member Author

@leighmcculloch I think I'll take your idea of just copying the current signing logic from the RPC. It should be pretty easy since the signing now happens through the config, so the callers wouldn't need to be updated. Will work on that now.

@willemneal willemneal force-pushed the feat/signer_and_rpc branch from 938bf32 to d960dd4 Compare June 7, 2024 20:33
@willemneal willemneal requested a review from leighmcculloch June 7, 2024 20:34
@willemneal willemneal force-pushed the feat/signer_and_rpc branch from 5de05af to b7f4163 Compare June 7, 2024 20:37
@willemneal willemneal enabled auto-merge (squash) June 7, 2024 20:37
@willemneal
Copy link
Member Author

@leighmcculloch This should be good for now and I'll add all the rest in a separate PR.

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.

Thanks. The change as a whole looks more like a copy-paste from stellar/stellar-rpc@587a229.

Couple questions inline but I think this is largely ready to merge.

cmd/soroban-cli/src/commands/contract/invoke.rs Outdated Show resolved Hide resolved
cmd/soroban-cli/src/commands/config/mod.rs Outdated Show resolved Hide resolved
@willemneal willemneal force-pushed the feat/signer_and_rpc branch from 4b7829c to 7a06130 Compare June 11, 2024 02:11
@willemneal willemneal changed the title feat: add Stellar signer trait and update to newest soroban-rpc feat: update to newest soroban-rpc and copy over old signing logic Jun 13, 2024
@janewang janewang self-requested a review June 14, 2024 13:27
@janewang janewang requested a review from chadoh June 14, 2024 17:18
Copy link
Contributor

@chadoh chadoh 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! One small cleanup nit, but no need to address it prior to merging.

@willemneal willemneal merged commit fb6527f into stellar:main Jun 14, 2024
23 of 24 checks passed
@chadoh chadoh deleted the feat/signer_and_rpc branch June 14, 2024 17:46
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