Skip to content

Prefix frontend dispatch#422

Closed
achamayou wants to merge 15 commits intomasterfrom
prefix_frontend_dispatch
Closed

Prefix frontend dispatch#422
achamayou wants to merge 15 commits intomasterfrom
prefix_frontend_dispatch

Conversation

@achamayou
Copy link
Member

Working draft for prefix dispatch change with the cert mods pulled out.

@ghost
Copy link

ghost commented Oct 8, 2019

images

@ghost
Copy link

ghost commented Oct 8, 2019

images

// Licensed under the Apache 2.0 License.
#pragma once

#include "../node/rpc/jsonrpc.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: without the ../?

certs.push_back(std::move(the_cert));
}

void add(std::shared_ptr<tls::Cert> cert)
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this used for?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a leftover from the cert changes, will take it out.

j["name"] = to_string(i);
const auto response =
json::from_msgpack(conn->call("SmallBank_balance", j));
json::from_msgpack(conn->call("users/SmallBank_balance", j));
Copy link
Member

Choose a reason for hiding this comment

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

Should this prefixing be done in RpcTlsClient::call? We already have the appropriate prefix in the sni member of TlsClient

Copy link
Member Author

Choose a reason for hiding this comment

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

It depends how we want to test stuff, but it certainly could.


std::string get_method(const std::string& method)
{
return method.substr(method.find_last_of('/') + 1, method.size());
Copy link
Member

Choose a reason for hiding this comment

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

If method doesn't contain /, this returns the entire string (find_last_of returns npos aka max(size_t), +1 wraps to 0) - is this the intended behaviour? I think RpcEndpoint::get_actor should return the actor and the remaining method, rather than repeating a similar parse here.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's the idea once this disappears, the method becomes an input to process. This is only here to get stuff working with the existing code path/validate the idea.

{
// The hostname indicates the rpc class.
auto host = hostname();
send(jsonrpc::pack(rpc, pack.value()));
Copy link
Contributor

Choose a reason for hiding this comment

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

does this need to be re-packed, can we not just send data?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, this is the response "couldn't deserialised", the naming is slightly confusing, but it isn't the original RPC.

session_id(session_id)
{}

std::optional<jsonrpc::Pack> detect_pack(const std::vector<uint8_t>& input)
Copy link
Contributor

Choose a reason for hiding this comment

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

We should make sure that detect_pack() and unpack_json() are not duplicated from the ones in frontend.h before we merge this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, absolutely, I'm removing a load of code from the frontend at the moment.

@achamayou achamayou closed this Nov 1, 2019
@achamayou achamayou deleted the prefix_frontend_dispatch branch February 27, 2020 16:24
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