ibc forwarder #95
ibc forwarder #95
Conversation
There was a problem hiding this comment.
Nothing major, just some code suggestions, 3 questions tho:
- You never change the state to completed, on purpose?
- forwarder will never send a bank message on other chain? it will always be a ibc call?
- Seems like this only meant for a single message, so the forwarder will never be used twice?
- Only reviewed forwarder and packages, nothing else.
| let deposit_address = if let Some(addr) = deposit_address_query { | ||
| addr | ||
| } else { | ||
| return Err(NeutronError::Std( | ||
| StdError::not_found("Next contract is not ready for receiving the funds yet") | ||
| )) | ||
| }; |
There was a problem hiding this comment.
You can do:
| let deposit_address = if let Some(addr) = deposit_address_query { | |
| addr | |
| } else { | |
| return Err(NeutronError::Std( | |
| StdError::not_found("Next contract is not ready for receiving the funds yet") | |
| )) | |
| }; | |
| let Some(deposit_address) = deposit_address_query else { | |
| return Err(NeutronError::Std( | |
| StdError::not_found("Next contract is not ready for receiving the funds yet") | |
| )) | |
| }; |
Something nice I learned today, much cleaner and clearer.
| None => Ok(Response::default() | ||
| .add_attribute("method", "try_forward_funds") | ||
| .add_attribute("error", "no_ica_found") | ||
| ), |
There was a problem hiding this comment.
Shouldn't we error here? or revert state back to instantiated?
if we reached this state, and still don't have ICA, it means something went wrong, reverting to instantiate state, can fix this issue without admin intervention.
There was a problem hiding this comment.
Good point. In theory this should never happen as we only end up in ICACreated state in case sudo_open_ack callback executes this:
INTERCHAIN_ACCOUNTS.save(
deps.storage,
port_id,
&Some((
parsed_version.clone().address,
parsed_version.clone().controller_connection_id,
)),
)?;
CONTRACT_STATE.save(deps.storage, &ContractState::ICACreated)?;However looking at it again, it probably makes sense to revert state to Instantiated to force recreation of the ICA. Will add this.
| let ica = if let Some((addr, _)) = INTERCHAIN_ACCOUNTS.load(deps.storage, key)? { | ||
| Some(addr) | ||
| } else { | ||
| None | ||
| }; |
There was a problem hiding this comment.
I think you can use map here:
| let ica = if let Some((addr, _)) = INTERCHAIN_ACCOUNTS.load(deps.storage, key)? { | |
| Some(addr) | |
| } else { | |
| None | |
| }; | |
| let ica = INTERCHAIN_ACCOUNTS.load(deps.storage, key)?.map(|(addr, _)| addr); |
There was a problem hiding this comment.
Actually realized we should probably use may_load here to not error in case ICA creation hadn't been triggered yet, will update to that
| if let Ok(parsed_version) = parsed_version { | ||
| INTERCHAIN_ACCOUNTS.save( | ||
| deps.storage, | ||
| port_id, | ||
| &Some(( | ||
| parsed_version.clone().address, | ||
| parsed_version.clone().controller_connection_id, | ||
| )), | ||
| )?; | ||
| CONTRACT_STATE.save(deps.storage, &ContractState::ICACreated)?; | ||
| return Ok(Response::default().add_attribute("method", "sudo_open_ack")); | ||
| } | ||
| Err(StdError::generic_err("Can't parse counterparty_version")) |
There was a problem hiding this comment.
Same as above, just a bit more cleaner.
| if let Ok(parsed_version) = parsed_version { | |
| INTERCHAIN_ACCOUNTS.save( | |
| deps.storage, | |
| port_id, | |
| &Some(( | |
| parsed_version.clone().address, | |
| parsed_version.clone().controller_connection_id, | |
| )), | |
| )?; | |
| CONTRACT_STATE.save(deps.storage, &ContractState::ICACreated)?; | |
| return Ok(Response::default().add_attribute("method", "sudo_open_ack")); | |
| } | |
| Err(StdError::generic_err("Can't parse counterparty_version")) | |
| let Ok(parsed_version) = parsed_version else { | |
| return Err(StdError::generic_err("Can't parse counterparty_version")) | |
| } | |
| INTERCHAIN_ACCOUNTS.save( | |
| deps.storage, | |
| port_id, | |
| &Some(( | |
| parsed_version.clone().address, | |
| parsed_version.clone().controller_connection_id, | |
| )), | |
| )?; | |
| CONTRACT_STATE.save(deps.storage, &ContractState::ICACreated)?; | |
| Ok(Response::default().add_attribute("method", "sudo_open_ack")) |
| impl InstantiateMsg { | ||
| pub fn get_response_attributes(&self) -> Vec<Attribute> { | ||
| vec![ | ||
| Attribute::new("clock_address", &self.clock_address), | ||
| Attribute::new("remote_chain_connection_id", &self.remote_chain_connection_id), | ||
| Attribute::new("remote_chain_channel_id", &self.remote_chain_channel_id), | ||
| Attribute::new("remote_chain_denom", &self.denom), | ||
| Attribute::new("remote_chain_amount", &self.amount), | ||
| Attribute::new("ibc_transfer_timeout", self.ibc_transfer_timeout.to_string()), | ||
| Attribute::new("ica_timeout", self.ica_timeout.to_string()), | ||
| ] | ||
| } | ||
| } |
There was a problem hiding this comment.
I feel like it should be possible to write a macro that would extract struct field names and generate an array of attributes 🤔
| } | ||
|
|
||
| #[proc_macro_attribute] | ||
| pub fn covenant_deposit_address(metadata: TokenStream, input: TokenStream) -> TokenStream { |
There was a problem hiding this comment.
Why new package? why not just use the clock one and rename that one, there should be just 1 macro package.
There was a problem hiding this comment.
Fair, merged all macros into one package in incoming pr.
Indeed it doesn't reach completion yet. I think we can discuss on whether we will want to use ICQs or alternative methods for that. In terms of bank msg / ibc calls, I was thinking of having a separate contract for non-ibc forwarding. Native forwarder? Overall I imagine forwarders to be disposable. Only responsible for facilitating a single transfer and reaching completion after that is done. |
Base implementation for #86 .
Does not cover completion yet.
Introduces
covenant-utilsand depositor query macro.