-
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: add stellar ledger as signer #1627
base: main
Are you sure you want to change the base?
Conversation
@leighmcculloch Moved here |
2b7c74a
to
0aaf7c5
Compare
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.
This is looking great! 🎉 🎉 A couple of questions, but mostly just for my own understanding.
@@ -0,0 +1,7 @@ | |||
pub mod http_transport; |
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 http_transport
be under the emulator-tests
feature flag too?
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 meant to add a http-transport
feature. above it because some unit tests still needed it.
@@ -394,23 +394,61 @@ impl Cmd { | |||
.ok_or(Error::ArchiveUrlNotConfigured) | |||
} | |||
|
|||
fn resolve_address( | |||
#[allow(dead_code)] |
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.
do we need to include resolve_address
here if it's not being used?
|
||
// Resolve an account address to an account id. The address can be a | ||
// G-address or a key name (as in `stellar keys address NAME`). | ||
#[allow(dead_code)] |
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.
same here - do we need to keep this in here if it's dead code??
|
||
use crate::integration::util::{deploy_contract, DeployKind, HELLO_WORLD}; | ||
|
||
// #[test_case("nanos"; "when the device is NanoS")] |
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 wonder if we can uncomment these other device model test cases
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 was running into issues around the server locally. Since all the accounts share the same seed phrase it was acting as one account. I also tried updating the testcase to add an hd-path, but it was causes issues. So I just wanted one to work on CI and then was going to return to it.
} | ||
} | ||
|
||
impl std::str::FromStr for KeyName { |
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 think that it makes sense not to allow for key names to be "ledger*", but I'm wondering how we can add a ledger identity via the cli - is this possible? If not, i think we want to revise the doc comment in cmd/soroban-cli/src/commands/keys/mod.rs for Add.
What
replaces #1415
Why
[TODO: Why this change is being made. Include any context required to understand the why.]
Known limitations
[TODO or N/A]