-
Notifications
You must be signed in to change notification settings - Fork 326
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
Add support for Namada #3705
base: master
Are you sure you want to change the base?
Add support for Namada #3705
Conversation
….1-namada Update yuji 1.7.1 namada
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.
Great job!
Left some initial comments and questions
crates/relayer/src/chain/namada.rs
Outdated
} | ||
|
||
impl NamadaChain { | ||
fn config(&self) -> &CosmosSdkConfig { |
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.
Is this required since there is already the fn config()
implementation from the ChainEndpoint
https://github.com/informalsystems/hermes/pull/3705/files#diff-c8209ac0ce0499033b7cfc2981c4284a1ec525a3e6848bbf0a11856a011c9c2bR188?
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.
removed
crates/relayer/src/chain/namada.rs
Outdated
|
||
let mut rpc_client = HttpClient::new(config.rpc_addr.clone()) | ||
.map_err(|e| Error::rpc(config.rpc_addr.clone(), e))?; | ||
rpc_client.set_compat_mode(CompatMode::V0_37); |
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.
Shouldn't the compat mode be retrieved from the configuration https://github.com/informalsystems/hermes/blob/master/config.toml#L439? It could default to V0_37
with a warning
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.
fixed
} | ||
|
||
fn shutdown(self) -> Result<(), Error> { | ||
Ok(()) |
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.
Is there a reason this method doesn't send the Shutdown
to the Event Source?
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.
fixed
crates/relayer/src/chain/namada.rs
Outdated
} | ||
|
||
fn version_specs(&self) -> Result<Specs, Error> { | ||
unimplemented!() |
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.
Will this eventually be implemented?
crates/relayer/src/chain/namada.rs
Outdated
heights.push(height); | ||
} | ||
// the key is not for a consensus state | ||
Err(_) => continue, |
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.
Is this expected to happen often?
If not, a debug!
would be nice to have the error. If it happens often then using trace!
would be preferred.
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.
added debug!
crates/relayer/src/chain/namada.rs
Outdated
); | ||
crate::telemetry!(query, self.id(), "query_upgraded_client_state"); | ||
|
||
unimplemented!() |
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.
Will this eventually be implemented?
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.
implemented
crates/relayer/src/chain/namada.rs
Outdated
); | ||
crate::telemetry!(query, self.id(), "query_upgraded_consensus_state"); | ||
|
||
unimplemented!() |
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.
Will this eventually be implemented?
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.
implemented
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.
Some small nits for error upstreaming
.latest_block_time | ||
.to_string() | ||
.parse() | ||
.unwrap()) |
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.
The error should be upstreamed instead of calling unwrap()
if key.to_string().ends_with("clientState") { | ||
let client_id = | ||
storage::client_id(&key).map_err(|e| Error::query(e.to_string()))?; | ||
let client_id = ClientId::from_str(&client_id.to_string()).unwrap(); |
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.
The error should be upstreamed instead of calling unwrap()
let PrefixValue { key, value: _ } = prefix_value; | ||
match storage::consensus_height(&key) { | ||
Ok(h) => { | ||
let height = ICSHeight::new(h.revision_number(), h.revision_height()).unwrap(); |
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.
The error should be upstreamed instead of calling unwrap()
let port_id = port_id.as_ref().parse().unwrap(); | ||
let channel_id = channel_id.as_ref().parse().unwrap(); |
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.
The errors should be upstreamed instead of calling unwrap()
Closes: #XXX
Description
To transfer tokens between Namada and Cosmos-base chains.
ChainConfig
CosmosSdkConfig
for generating Tendermint light clientsNamadaChain
asChainEndpoint
implementationcrates/relayer/src/chain/namada.rs
and files undercrates/relayer/src/chain/namada
PR author checklist:
unclog
.docs/
).Reviewer checklist:
Files changed
in the GitHub PR explorer.