feat(bridge-withdrawer): add ics20 withdrawal timeout duration option to withdrawer #1719
Conversation
|
Is there a justification for this outside of testing? If the reason to get this in is testing timeouts then we should also provide some kind of tests - at least blackbox tests. |
|
@SuperFluffy main reason right now is testing, but it could be used normally, for example if someone deploys their own bridge and they want a custom/different ibc transfer timeout time. |
There was a problem hiding this comment.
LGTM (just a couple of nitpicks), but I agree with @SuperFluffy that it would be good to have test coverage of this new functionality.
| @@ -48,6 +48,9 @@ ASTRIA_BRIDGE_WITHDRAWER_ROLLUP_ASSET_DENOMINATION="nria" | |||
| # Should match the bridge address in the geth rollup's bridge configuration for that asset. | |||
| ASTRIA_BRIDGE_WITHDRAWER_SEQUENCER_BRIDGE_ADDRESS="" | |||
|
|
|||
| # The timeout duration for `Ics20Withdrawal`s in seconds. | |||
| ASTRIA_BRIDGE_WITHDRAWER_ICS20_WITHDRAWAL_TIMEOUT_DURATION=300 | |||
There was a problem hiding this comment.
Might make the units more visible if we named this ASTRIA_BRIDGE_WITHDRAWER_ICS20_WITHDRAWAL_TIMEOUT_SECONDS?
| @@ -26,6 +26,8 @@ pub struct Config { | |||
| pub rollup_asset_denomination: asset::denom::TracePrefixed, | |||
| // The bridge address corresponding to the bridged rollup asset on the sequencer. | |||
| pub sequencer_bridge_address: String, | |||
| // The timeout duration for `Ics20Withdrawal`s in seconds. | |||
| pub ics20_withdrawal_timeout_duration: u64, | |||
There was a problem hiding this comment.
Likewise, since this isn't a Duration yet, renaming to ics20_withdrawal_timeout_seconds might be slightly clearer?
Summary
add ics20 withdrawal timeout duration option to withdrawer.
Background
potentially useful for testing the timeout path of ics20 transfers.
Changes
Testing
existing unit tests; can't really test this fully without an actual ibc connection/smoke test setup