fix: review sov-ibc-transfer implementation and apply fixes #133
Conversation
Farhad-Shabani
changed the title
escrowed_token + move away from base64-encoded memoescrowed_token + move away from base64-encoded memo
| if !memo.as_ref().is_empty() { | ||
| return Err(TokenTransferError::Other( | ||
| "Memo must be empty when burning tokens".to_string(), | ||
| )); | ||
| } |
There was a problem hiding this comment.
Can you please explain why this is required?
There was a problem hiding this comment.
Just a safety check and disallowing someone to pass a huge memo that might overload the system. This perhaps should be taken care at ibc-rs ( I mean the memo size), then we can drop it here.
There was a problem hiding this comment.
Ah yes. But this may prevent users from using the ibc-middleware hooks in counterparty chains.
There was a problem hiding this comment.
Can you please add some comments about this situation?
Farhad-Shabani
changed the title
escrowed_token + move away from base64-encoded memo
There was a problem hiding this comment.
I added some last comments. Otherwise, the PR looks solid 🪨
Thanks, @Farhad-Shabani ! ✨
| if !memo.as_ref().is_empty() { | ||
| return Err(TokenTransferError::Other( | ||
| "Memo must be empty when burning tokens".to_string(), | ||
| )); | ||
| } |
There was a problem hiding this comment.
Can you please add some comments about this situation?
Closes: #132
Description
Along with this PR:
escrowed_tokenstate field.TokenId, we can directly get itsStringrepresentation, eliminating the need to rely onmemo.minted_token_id_to_nameto disallow using theTokenIdof an IBC-minted token as the denomination for sending back to the source chain. (Brought up in discussions with @rnbguy)ibc_transferaddress as the sender.PR author checklist
docs/).