-
Notifications
You must be signed in to change notification settings - Fork 76
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
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 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.)
@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. |
938bf32
to
d960dd4
Compare
5de05af
to
b7f4163
Compare
@leighmcculloch This should be good for now and I'll add all the rest in a separate PR. |
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.
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.
4b7829c
to
7a06130
Compare
stellar-rpc-client
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.
Looks good! One small cleanup nit, but no need to address it prior to merging.
What
Add a Stellar trait for signing (up for suggestions for a better nameSigner
seemed overloaded). This will allow for other implementations like the Ledger in a future PR. An initial implementation of the traitLocalKey
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]