pcli: use a default timeout height for ibc withdrawals #3558
Conversation
Added in #3572 |
crates/bin/pcli/src/command/tx.rs
Outdated
| IbcChannelQueryClient::new(app.pd_channel().await?); | ||
|
|
||
| let req = QueryChannelRequest { | ||
| port_id: "transfer".to_string(), |
There was a problem hiding this comment.
Side note: we hardcode "transfer" a lot of places, when referring to the IBC port. Should we not constify that value in the IBC crate somewhere, so we can tersely refer to it as the constant value it is?
There was a problem hiding this comment.
We do this, it's Portid::transfer() in ibc-types. i'll update this pr to use that
| Some(h) => h.clone(), | ||
| None => { | ||
| // look up the height for the counterparty and add 2 days of block time | ||
| // (assuming 10 seconds per block) to it |
There was a problem hiding this comment.
Why assume 10s per block? Looking at the current Testnet 64, it looks like blocks actually take about 5s:
which I assume is coming from the cometbft value we set here: https://github.com/penumbra-zone/penumbra/blob/v0.64.1/testnets/cometbft_config_template.toml#L413
There was a problem hiding this comment.
The number applies to counterparty chains, so we can't get it correct, since they all have different values, it'll just be an approximation. See
https://twitter.com/larry0x/status/1742764484144320997/photo/1
all the cosmos chains have different block times
There was a problem hiding this comment.
Ahh, of course. That makes a lot more sense!
crates/bin/pcli/src/command/tx.rs
Outdated
|
|
||
| IbcHeight { | ||
| revision_number: last_update_height.revision_number, | ||
| revision_height: last_update_height.revision_height + 87400, |
There was a problem hiding this comment.
Related to question above: where does 87400 come from? 24 * 60 * 60 = 86400 but the comment at the top of the match suggests it should be two days, not one. Can we var this value and explain its construction in a comment?
There was a problem hiding this comment.
it should be ((246060)/10) * 2, since we're assuming 10s per block
|
Updated with review comments |
avahowell
force-pushed
the
withdraw-default-height
branch
from
efdf20b to
9642054
Compare
|
Awesome, thanks for explaining, @avahowell! |
Closes #3206 . Won't work until we have implemented the remaining IBC queries (currently all the channel ones are missing)