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

generate new Sender if hd_index was provided in set_options #404

Merged

Conversation

Buckram123
Copy link
Contributor

hd_index impacts private key meaning if it's provided inside SenderOptions new Sender should be created from scratch
Closes ORC-127, Closes ORC-125

@Buckram123 Buckram123 requested a review from Kayanski May 28, 2024 13:40
@Buckram123 Buckram123 changed the title generate new sender if hd_index was provided in set_options generate new Sender if hd_index was provided in set_options May 28, 2024
Copy link

cloudflare-workers-and-pages bot commented May 28, 2024

Deploying cw-orchestrator with  Cloudflare Pages  Cloudflare Pages

Latest commit: 9215edd
Status: ✅  Deploy successful!
Preview URL: https://ecec2f8a.cw-orchestrator.pages.dev
Branch Preview URL: https://misha-orc-127-daemon-rebuild.cw-orchestrator.pages.dev

View logs

Copy link

codecov bot commented May 28, 2024

Codecov Report

Attention: Patch coverage is 93.33333% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 65.1%. Comparing base (41fc930) to head (9215edd).

Additional details and impacted files
Files Coverage Δ
cw-orch-daemon/src/keys/private.rs 81.1% <100.0%> (+1.8%) ⬆️
cw-orch-daemon/src/sync/builder.rs 94.1% <100.0%> (+7.2%) ⬆️
cw-orch-daemon/src/sender.rs 62.9% <85.7%> (+3.2%) ⬆️

... and 2 files with indirect coverage changes

.build()
.unwrap();

let indexed_daemon = daemon.rebuild().hd_index(56).build().unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing the hd_index doesn't change the sender address?

Copy link
Contributor Author

@Buckram123 Buckram123 Jun 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On rebuild - yes, that's what this PR tries to fix

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then why are you asserting that the address basically remains the same? This test confuses me a bit

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assert_ne is not equal, before this PR they eq, and now they ne

Copy link
Contributor

@Kayanski Kayanski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice !
We will need to support updating the daemon state as well when rebuilding, but that's another task.

@CyberHoward
Copy link
Contributor

Very nice ! We will need to support updating the daemon state as well when rebuilding, but that's another task.

What do you mean by this?

@Kayanski
Copy link
Contributor

Kayanski commented Jun 4, 2024

Very nice ! We will need to support updating the daemon state as well when rebuilding, but that's another task.

What do you mean by this?

Here, when the state of the daemon is updated (grpc channel for instance, if a same daemon is used for another chain), the sender channel is not updated which leads to issues if you're changing the chain.

https://github.com/AbstractSDK/cw-orchestrator/blob/main/cw-orch-daemon/src/builder.rs#L110

This is addressed in #326

@Kayanski Kayanski merged commit 9c43514 into main Jun 4, 2024
18 checks passed
@Kayanski Kayanski deleted the misha/orc-127-daemon-rebuild-hd_index-does-not-generate-new-key branch June 4, 2024 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants