Skip to content
This repository has been archived by the owner on Feb 3, 2023. It is now read-only.

Node-to-node send/receive #746

Merged
merged 35 commits into from
Dec 13, 2018
Merged

Node-to-node send/receive #746

merged 35 commits into from
Dec 13, 2018

Conversation

lucksus
Copy link
Collaborator

@lucksus lucksus commented Dec 11, 2018

Usage

Define receive callback in define_zome! next to genesis:

define_zome! {
...
    genesis: || { Ok(()) }

    receive: |payload| {
        format!("Received: {}", payload)
    }

    functions: {

Use hdk::send to asynchronously send and receive the return value of the receive callback from the other node:

fn handle_send_message(to_agent: Address, message: String) -> String {
    match hdk::send(to_agent, message) {
        Ok(response) => response,
        Err(error) => error.to_string(),
    }
}

Yes, you can have a receive callback in each zome. hdk::send calls only map to the receive callback of the zome in which send gets called.

Chunks

  • receive added to define_zome! macro
  • CustomDirectMessage added
  • hc_send/invoke_send implemented
  • custom_send action creator and Future that waits for the result to appear in the (network) state
  • handle_custom_direct_message workflow that runs on the receiving end, calls the callback and dispatches an action to send the response

@zippy
Copy link
Member

zippy commented Dec 12, 2018

super woot, but tests didn't pass :-(

@codecov
Copy link

codecov bot commented Dec 12, 2018

Codecov Report

Merging #746 into develop will decrease coverage by 1.12%.
The diff coverage is 22.34%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #746      +/-   ##
===========================================
- Coverage    73.31%   72.19%   -1.11%     
===========================================
  Files          138      142       +4     
  Lines         4690     4789      +99     
===========================================
+ Hits          3438     3457      +19     
- Misses        1252     1332      +80
Impacted Files Coverage Δ
core/src/action.rs 100% <ø> (ø) ⬆️
hdk-rust/src/api.rs 0% <ø> (ø) ⬆️
core/src/nucleus/ribosome/callback/mod.rs 67.17% <0%> (ø) ⬆️
...rc/network/reducers/handle_custom_send_response.rs 0% <0%> (ø)
core/src/network/reducers/mod.rs 95.13% <0%> (-2.43%) ⬇️
core/src/network/reducers/send_direct_message.rs 78.95% <0%> (-9.28%) ⬇️
core/src/network/actions/custom_send.rs 0% <0%> (ø)
core/src/nucleus/ribosome/api/send.rs 0% <0%> (ø)
core/src/workflows/handle_custom_direct_message.rs 0% <0%> (ø)
core/src/network/state.rs 100% <100%> (ø) ⬆️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 24a32f4...77444f3. Read the comment docs.

@codecov
Copy link

codecov bot commented Dec 12, 2018

Codecov Report

Merging #746 into develop will decrease coverage by 1.05%.
The diff coverage is 28.58%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #746      +/-   ##
===========================================
- Coverage    73.76%   72.72%   -1.04%     
===========================================
  Files          141      145       +4     
  Lines         4733     4842     +109     
===========================================
+ Hits          3491     3521      +30     
- Misses        1242     1321      +79
Impacted Files Coverage Δ
core/src/action.rs 100% <ø> (ø) ⬆️
hdk-rust/src/api.rs 0% <ø> (ø) ⬆️
core/src/workflows/handle_custom_direct_message.rs 0% <0%> (ø)
...rc/network/reducers/handle_custom_send_response.rs 0% <0%> (ø)
hdk-rust/src/error.rs 0% <0%> (ø) ⬆️
core/src/network/actions/custom_send.rs 0% <0%> (ø)
core/src/nucleus/ribosome/callback/mod.rs 67.17% <0%> (ø) ⬆️
core/src/nucleus/ribosome/api/send.rs 0% <0%> (ø)
core/src/network/reducers/send_direct_message.rs 96% <100%> (+7.77%) ⬆️
core/src/network/state.rs 100% <100%> (ø) ⬆️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0c669e7...5a05493. Read the comment docs.

Copy link
Member

@zippy zippy left a comment

Choose a reason for hiding this comment

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

I approve this because changes needed are minor (comment updates mostly)

However, there's one thing missing here which we had in proto which is timeout. Currently this blocks for ever, and we need to allow setting timeout as a followup.

core/src/network/actions/custom_send.rs Outdated Show resolved Hide resolved
core/src/network/actions/custom_send.rs Outdated Show resolved Hide resolved
core/src/network/direct_message.rs Outdated Show resolved Hide resolved
core/src/nucleus/ribosome/api/send.rs Outdated Show resolved Hide resolved
core/src/workflows/handle_custom_direct_message.rs Outdated Show resolved Hide resolved
hdk-rust/tests/integration_test.rs Outdated Show resolved Hide resolved
@lucksus
Copy link
Collaborator Author

lucksus commented Dec 12, 2018

Implemented all your points, @zippy. Yes, the timeout, was thinking about this and planning to add it as follow up. Or should I add it here and now?

@zippy
Copy link
Member

zippy commented Dec 12, 2018

I'm ok with followup unless you think it's really easy.

One other thing I forgot was that there's no app_spec example. That should also be a followup.

@@ -333,6 +333,13 @@ fn hdk_test_entry() -> Entry {
Entry::App(hdk_test_app_entry_type(), hdk_test_entry_value())
}

fn handle_send_message(to_agent: Address, message: String) -> String {
Copy link
Collaborator

Choose a reason for hiding this comment

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

So send and receive can pass/return String, can it use other types?

Like could receive return a ZomeApiResult?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried with a Result<String, String> but discovered a bug in JsonString::from<Result<>>. Already synced with @thedavidmeister. So I reverted back to just String for now.

Basically possible but tried to keep the scope small for this one.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see, ok well knowing that we will implement a complex struct, not just a string eases my mind on this one 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

oh yeah i forgot about that bug

i'd better actually fix that! 😅

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a follow up created for this one @lucksus @thedavidmeister ?

/// This is direct message that got created by the zome code through hdk::send().
#[derive(Serialize, Deserialize, Clone, PartialEq, Debug, DefaultJson)]
pub struct CustomDirectMessage {
/// We have to track which zome sent the message so we can call the
Copy link
Collaborator

Choose a reason for hiding this comment

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

That's an interesting point... so an app can have one receive callback per zome?

But it could handle different event types, if they were passed as data, and using a match in receive?

Copy link
Contributor

Choose a reason for hiding this comment

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

@Connoropolous pretty similar to how websockets works i guess, you can send and receive strings, but dispatching logic and simulating request/response behaviour is BYO


let zome_call = ZomeFnCall::new(
zome,
"no capability since this is a callback",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I thought we weren't calling this capability anymore? Trait?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We haven't removed "capability" from the code yet. Otherwise this function wouldn't expect a capability name as second parameter.

Copy link
Collaborator

Choose a reason for hiding this comment

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

K

/// [define_zome!](macro.defineZome.html) macro.
///
/// This functions blocks and returns the result returned by the `receive` callback on
/// the other side.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I want to add example code in here to this description, but I am open to / happy to do that as a followup

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Great 👍

@lucksus
Copy link
Collaborator Author

lucksus commented Dec 12, 2018

@zippy, timeout added: 5b50169

Co-Authored-By: lucksus <nicolas@lucksus.org>
@lucksus lucksus merged commit dda6f96 into develop Dec 13, 2018
@lucksus lucksus removed the review label Dec 13, 2018
@Connoropolous
Copy link
Collaborator

Wooot!

@lucksus lucksus deleted the send-receive branch December 14, 2018 13:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants