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

1.7.0 with Namada #14

Closed
wants to merge 21 commits into from
Closed

1.7.0 with Namada #14

wants to merge 21 commits into from

Conversation

yito88
Copy link
Member

@yito88 yito88 commented Nov 8, 2023

closes #11

  • Add Namada to ChainConfig
    • Reuse CosmosSdkConfig for generating Tendermint light clients
  • Add NamadaChain as ChainEndpoint implementation
    • See crates/relayer/src/chain/namada.rs and files under crates/relayer/src/chain/namada
    • To submit Namada transactions and query
  • Store Namada key to KeyRing Add Namada key to the store #15

@yito88 yito88 force-pushed the yuji/1.7.0-namada branch from 3758262 to 665cec3 Compare November 8, 2023 12:55
@yito88 yito88 marked this pull request as ready for review November 9, 2023 14:21
@yito88 yito88 changed the title [draft] 1.7.0 with Namada 1.7.0 with Namada Nov 10, 2023
@yito88 yito88 requested review from sug0 and cwgoes November 11, 2023 09:30
Copy link

@cwgoes cwgoes left a comment

Choose a reason for hiding this comment

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

Just took a quick look for now. Do you know if any other non-Cosmos-SDK chains have been added to upstream Hermes yet? Do they have any guidelines for new chains who want to submit PRs?

@@ -20,7 +20,11 @@ jobs:
with:
toolchain: nightly-2023-07-13
override: true

- name: Install Protoc
Copy link

Choose a reason for hiding this comment

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

do we really need to ask informal to make this a dependency?

Copy link
Member Author

Choose a reason for hiding this comment

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

We can't build Hermes importing namada_sdk and namada without protoc.

let response = self.send_tx(msg)?;

// Note: we don't have any height information in this case. This hack will fix itself
// once we remove the `ChainError` event (which is not actually an event)
Copy link

Choose a reason for hiding this comment

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

is this our hack or informal's hack?

Copy link
Member Author

Choose a reason for hiding this comment

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

@yito88
Copy link
Member Author

yito88 commented Nov 11, 2023

Do you know if any other non-Cosmos-SDK chains have been added to upstream Hermes yet?

There are only Cosmos-SDK chains using CosmosSdkChain in the upstream. NamadaChain is the first one of non-Cosmos-SDK.

Do they have any guidelines for new chains who want to submit PRs?

They said we could open a PR for Namada. I will share this progress and ask them about it again.

Copy link

@sug0 sug0 left a comment

Choose a reason for hiding this comment

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

just a few nits/suggestions

.map_err(|_| eyre!("error loading Namada wallet"))?;

let secret_key = wallet
.find_secret_key(key_name, None)
Copy link

Choose a reason for hiding this comment

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

passing None here will interrupt the process with a prompt, right? should we instead hand out the password via a secrets file or smth?

Ok(monitor_tx)
}

fn get_unbonding_time(&self) -> Result<Duration, Error> {
Copy link

Choose a reason for hiding this comment

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

Suggested change
fn get_unbonding_time(&self) -> Result<Duration, Error> {
fn get_min_unbonding_time(&self) -> Result<Duration, Error> {

maybe? since this is not a precise duration (epochs can last longer than epoch_duration.min_duration)

Comment on lines +226 to +233
store.insert_keypair::<wallet::NullWalletUtils>(
config.key_name.clone().into(),
key.secret_key,
None,
Some(key.address),
None,
true,
);
Copy link

Choose a reason for hiding this comment

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

this won't encrypt the keys right? maybe we should add an option to do so, later on

@Fraccaman Fraccaman closed this Sep 25, 2024
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.

Move back to mainstream tendermint-rs
4 participants