Skip to content
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

Callbacks #75

Merged
merged 9 commits into from
Dec 13, 2023
Merged

Conversation

Art3miX
Copy link
Collaborator

@Art3miX Art3miX commented Nov 29, 2023

Callback implementation for ICS721.

There are 2 types of callbacks:

  1. Receive callback: callback that is called after a successful transfer on the receiving chain.
  2. Ack callback: callback that is called after a transfer was acknowledged, on the sending chain.

Use cases:

  1. Users can send NFTs to a contract and tell the contract to do something, using the receive callback
  2. Contracts can send NFTs, get the status of the transfer and act upon failed/successful transfers, using the ack callback
  3. platforms/systems can have a complex flow of contracts where 1 contract updates the other of a transfer directly.

ICS721-tester is a contract used in tests to confirm the data the callback is seeing/getting is the correct data we expect.
It is also a good reference for how a contract would implement accepting callbacks.

@Art3miX Art3miX requested a review from taitruong November 29, 2023 10:46
/// status = Ics721Status::Failed - Transfer failed and contract still owns the NFT
#[cw_serde]
pub struct Ics721AckCallbackMsg {
pub status: Ics721Status,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case of Ics721Status::Failed there are 2 cases: error on target chain (e.g. from a contract) or because of IBC timeout. In both cases there is an error string. So we could add an error: Option<String> here. This field is None in case of success.
This way ack callback knows the reason of fail and may handle both cases.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, we can just add the error message to the Error case, Error(String), this way if its an error you would get an error.
The Failed case is called for both timeout and failed on the other side, so just a message there should be enough.

@shanev
Copy link
Member

shanev commented Dec 1, 2023

This is amazing!

@taitruong
Copy link
Collaborator

This PR has internally been reviewed here: arkprotocol#13
LGTM.
Only minor suggestion is adding error to Ics721AckCallbackMsg. Check my previous comment.

Comment on lines +100 to +113
let msg = to_json_binary(&ReceiverExecuteMsg::Ics721ReceiveCallback(
Ics721ReceiveCallbackMsg {
msg: receive_callback_data,
nft_contract,
original_packet: packet.clone(),
},
))
.ok()?;

Some(WasmMsg::Execute {
contract_addr,
msg,
funds: vec![],
})
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got feedback from Vlad, TFL: would be great if msg (instead of data) can be passed in memo and being used directly for execution. Use case here would be calling SendNft on cw721 contract.

Imo, this would be a lot more flexible - without other contracts being adjusted. All we need to make sure is that msg is not called on ICS721.

@humanalgorithm humanalgorithm self-requested a review December 11, 2023 07:55
@taitruong taitruong added this pull request to the merge queue Dec 13, 2023
Merged via the queue into public-awesome:main with commit 3daacae Dec 13, 2023
1 check passed
@taitruong taitruong deleted the art3mix/updated-callbacks branch June 10, 2024 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants