Merge pull request #290 from icon-project/bugfix/xcall-java-empty-res…

…ponse-decoding fix: Add support for non null reply encoding
icon-project · Apr 8, 2024 · f14e392 · f14e392
2 parents 617016e + 94be8b5
commit f14e392
Show file tree

Hide file tree

Showing 5 changed files with 159 additions and 12 deletions.
diff --git a/contracts/cosmwasm-vm/cw-integration/tests/test_xcall.rs b/contracts/cosmwasm-vm/cw-integration/tests/test_xcall.rs
@@ -358,3 +358,137 @@ fn test_rollback_reply() {
     println!("{event:?}");
     assert_eq!(&expected_hex, event.get("data").unwrap());
 }
+
+fn test_call_message(
+    mut ctx: &mut TestContext,
+    data: Vec<u8>,
+    msg_type: MessageType,
+) -> Result<AppResponse, AppError> {
+    call_set_xcall_host(ctx).unwrap();
+    call_register_connection(ctx).unwrap();
+    let src = ctx.get_xcall_ibc_connection().to_string();
+    let _dapp = ctx.get_dapp().to_string();
+
+    let nid = "0x3.icon";
+    call_configure_connection(
+        ctx,
+        "connection-1".to_string(),
+        nid.to_string(),
+        "client-1".to_string(),
+    )
+    .unwrap();
+    call_ibc_channel_connect(ctx).unwrap();
+    call_dapp_add_connection(ctx, src, "somedest".to_string(), nid.to_string()).unwrap();
+    let msg = CSMessageRequest::new(
+        NetworkAddress::from_str(&format!("{nid}/{MOCK_CONTRACT_TO_ADDR}")).unwrap(),
+        ctx.get_dapp(),
+        1,
+        msg_type,
+        data.clone(),
+        vec![ctx.get_xcall_ibc_connection().to_string()],
+    );
+    let request = CSMessage {
+        message_type: cw_xcall::types::message::CSMessageType::CSMessageRequest,
+        payload: msg.as_bytes(),
+    };
+
+    let msg = Message {
+        sn: Nullable::new(Some(1_i64)),
+        fee: 0_u128,
+        data: request.as_bytes(),
+    };
+    let bytes: Vec<u8> = common::rlp::encode(&msg).to_vec();
+
+    call_ibc_receive_packet(ctx, bytes).unwrap();
+    call_execute_call_message(ctx, 1, data)
+}
+
+#[test]
+fn test_call_message_failed() {
+    let mut ctx = setup_test();
+
+    let data = "rollback".as_bytes().to_vec();
+    let resp = test_call_message(&mut ctx, data, MessageType::CallMessage);
+    assert!(resp.is_ok());
+
+    let event = get_event(&resp.unwrap(), "wasm-CallExecuted").unwrap();
+    let expected_code: u8 = CallServiceResponseType::CallServiceResponseFailure.into();
+    assert_eq!(event.get("code").unwrap(), &expected_code.to_string());
+}
+
+#[test]
+fn test_call_message_success() {
+    let mut ctx = setup_test();
+
+    let data = "test".as_bytes().to_vec();
+    let resp = test_call_message(&mut ctx, data, MessageType::CallMessage);
+    assert!(resp.is_ok());
+    let result = resp.unwrap();
+    let event = get_event(&result, "wasm-CallExecuted").unwrap();
+    let ack_event = get_event(&result, "wasm-write_acknowledgement");
+    assert!(ack_event.is_none());
+
+    let expected_code: u8 = CallServiceResponseType::CallServiceResponseSuccess.into();
+    assert_eq!(event.get("code").unwrap(), &expected_code.to_string());
+}
+
+#[test]
+#[should_panic(expected = "NotFound { kind: \"cw_xcall::types::request::CSMessageRequest\"")]
+fn test_call_message_re_execute() {
+    let mut ctx = setup_test();
+
+    let data = "rollback".as_bytes().to_vec();
+    let resp = test_call_message(&mut ctx, data.clone(), MessageType::CallMessage);
+    assert!(resp.is_ok());
+    // CallRequest should have been removed even though call failed
+    let _ = call_execute_call_message(&mut ctx, 1, data);
+}
+
+#[test]
+fn test_persistent_call_message_success() {
+    let mut ctx = setup_test();
+
+    let data = "test".as_bytes().to_vec();
+    let resp = test_call_message(&mut ctx, data, MessageType::CallMessagePersisted);
+    assert!(resp.is_ok());
+
+    let result = resp.unwrap();
+    let event = get_event(&result, "wasm-CallExecuted").unwrap();
+    let ack_event = get_event(&result, "wasm-write_acknowledgement");
+    assert!(ack_event.is_none());
+
+    let expected_code: u8 = CallServiceResponseType::CallServiceResponseSuccess.into();
+    assert_eq!(event.get("code").unwrap(), &expected_code.to_string());
+}
+
+#[test]
+#[should_panic(expected = "NotFound { kind: \"cw_xcall::types::request::CSMessageRequest\"")]
+fn test_persistent_call_message_re_execute() {
+    let mut ctx = setup_test();
+
+    let data = "test".as_bytes().to_vec();
+    let resp = test_call_message(&mut ctx, data.clone(), MessageType::CallMessagePersisted);
+    assert!(resp.is_ok());
+
+    let result = resp.unwrap();
+    let event = get_event(&result, "wasm-CallExecuted").unwrap();
+
+    let expected_code: u8 = CallServiceResponseType::CallServiceResponseSuccess.into();
+    assert_eq!(event.get("code").unwrap(), &expected_code.to_string());
+
+    // removed after a successful execution
+    let _ = call_execute_call_message(&mut ctx, 1, data);
+}
+
+#[test]
+fn test_persistent_call_message_retry() {
+    let mut ctx = setup_test();
+
+    let data = "rollback".as_bytes().to_vec();
+    let resp = test_call_message(&mut ctx, data.clone(), MessageType::CallMessagePersisted);
+    assert!(resp.is_err());
+
+    // can retry
+    let resp = call_execute_call_message(&mut ctx, 1, data);
+    assert!(resp.is_err());
+}
diff --git a/contracts/cosmwasm-vm/cw-xcall/src/execute_call.rs b/contracts/cosmwasm-vm/cw-xcall/src/execute_call.rs
@@ -76,21 +76,21 @@ impl<'a> CwCallService<'a> {
         self.remove_execute_request_id(deps.storage);
 
         let request = self.get_proxy_request(deps.storage, req_id)?;
+        self.remove_proxy_request(deps.storage, req_id);
+        let reply = self
+            .pop_call_reply(deps.storage)
+            .map(|msg| rlp::encode(&msg).to_vec());
 
         let (response, event) = match msg.result {
             cosmwasm_std::SubMsgResult::Ok(_res) => {
                 let code = CallServiceResponseType::CallServiceResponseSuccess.into();
-                let reply = self
-                    .pop_call_reply(deps.storage)
-                    .map(|msg| rlp::encode(&msg).to_vec());
                 let message_response = CSMessageResult::new(
                     request.sequence_no(),
                     CallServiceResponseType::CallServiceResponseSuccess,
                     reply,
                 );
 
                 let event = event_call_executed(req_id, code, "success");
-                self.remove_proxy_request(deps.storage, req_id);
                 (message_response, event)
             }
             cosmwasm_std::SubMsgResult::Err(err) => {

diff --git a/contracts/cosmwasm-vm/cw-xcall/tests/test_handle_call_message.rs b/contracts/cosmwasm-vm/cw-xcall/tests/test_handle_call_message.rs
@@ -310,12 +310,7 @@ fn test_persisted_message_not_removed_on_error() {
 
     let _response = contract.reply(mock_deps.as_mut(), env, msg);
 
-    assert_eq!(_response.is_err(), true);
-
-    let req = contract
-        .get_proxy_request(mock_deps.as_ref().storage, request_id)
-        .unwrap();
-    assert_eq!(req, proxy_requests);
+    assert!(_response.is_err());
 }
 
 #[test]

diff --git a/contracts/javascore/xcall/src/main/java/foundation/icon/xcall/CallServiceImpl.java b/contracts/javascore/xcall/src/main/java/foundation/icon/xcall/CallServiceImpl.java
@@ -502,7 +502,7 @@ private void handleResult(byte[] data) {
         switch (msgRes.getCode()) {
             case CSMessageResult.SUCCESS:
                 cleanupCallRequest(resSn);
-                if (msgRes.getMsg() != null) {
+                if (msgRes.getMsg() != null && msgRes.getMsg().length > 0) {
                     handleReply(rollback, CSMessageRequest.fromBytes(msgRes.getMsg()));
                 }
                 successfulResponses.set(resSn, true);

diff --git a/contracts/javascore/xcall/src/test/java/foundation/icon/xcall/CallServiceTest.java b/contracts/javascore/xcall/src/test/java/foundation/icon/xcall/CallServiceTest.java
@@ -349,6 +349,24 @@ public void handleRequest_same_network_id() {
         verify(xcallSpy, times(0)).CallMessage(anyString(), anyString(), any(BigInteger.class), any(BigInteger.class), any(byte[].class));
     }
 
+    @Test
+    public void handleResult_evmEncoding() {
+        // Arrange
+        xcall.invoke(owner, "setDefaultConnection", ethDapp.net(), baseConnection.getAddress());
+
+        byte[] data = "test".getBytes();
+        CSMessageResult result = new CSMessageResult(BigInteger.ONE, CSMessageResult.SUCCESS, new byte[0]);
+        CSMessage msg = new CSMessage(CSMessage.RESULT, result.toBytes());
+
+        xcall.invoke(dapp.account, "sendCallMessage", ethDapp.toString(), data, data, baseSource, baseDestination);
+
+        // Act
+        xcall.invoke(baseConnection.account, "handleMessage", ethNid, msg.toBytes());
+
+        // Assert
+        verify(xcallSpy).ResponseMessage(BigInteger.ONE, CSMessageResult.SUCCESS);
+    }
+
     @Test
     public void handleReply() {
         // Arrange
@@ -369,7 +387,7 @@ public void handleReply() {
         verify(xcallSpy).CallMessage(ethDapp.toString(), dapp.getAddress().toString(), BigInteger.ONE, BigInteger.ONE, data);
     }
 
-     @Test
+    @Test
     public void handleReply_invalidTo() {
         // Arrange
         xcall.invoke(owner, "setDefaultConnection", ethDapp.net(), baseConnection.getAddress());