Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Permissioned p2p connections #6359

Merged
merged 1 commit into from
Aug 29, 2017
Merged

Permissioned p2p connections #6359

merged 1 commit into from
Aug 29, 2017

Conversation

arkpar
Copy link
Collaborator

@arkpar arkpar commented Aug 23, 2017

No description provided.

@arkpar arkpar added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Aug 23, 2017
@keorn keorn added the B7-releasenotes 📜 Changes should be mentioned in the release notes of the next minor version release. label Aug 23, 2017
@arkpar arkpar force-pushed the node-contract branch 2 times, most recently from 87790a4 to cc374da Compare August 23, 2017 09:53
/// Connection filter that uses a contract to manage permissions.
pub struct NodeFilter {
contract: Mutex<Option<Contract>>,
client: Weak<Client>,
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason why not a Weak<BlockChainClient>?
light client support probably doesn't make sense for this, since generally light clients only initiate outgoing connections as opposed to accepting incoming

fn connection_allowed(&self, own_id: &NodeId, connecting_id: &NodeId, _direction: ConnectionDirection) -> bool {

let mut cache = self.permission_cache.lock();
if let Some(res) = cache.get(connecting_id) {
Copy link
Contributor

Choose a reason for hiding this comment

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

should it be memory bounded somehow?

#[cfg(test)]
mod test {

use std::sync::Arc;
Copy link
Contributor

Choose a reason for hiding this comment

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

tests module code needs indenting


let mut contract = self.contract.lock();
if contract.is_none() {
*contract = Some(Contract::new(self.contract_address));
Copy link
Contributor

Choose a reason for hiding this comment

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

i don't see the purpose of this. we have the address at initialization, so why not just create the contract structure there?

Copy link
Contributor

Choose a reason for hiding this comment

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

i guess it just defers ABI-decoding a bit?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

right, just deferred initialization that could save some time in offline mode.

@rphmeier
Copy link
Contributor

Grumbles about contract wrapper, otherwise LGTM

@rphmeier rphmeier added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Aug 24, 2017
@arkpar arkpar added A0-pleasereview 🤓 Pull request needs code review. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Aug 28, 2017
"0000000000000000000000000000000000000004": { "balance": "1", "builtin": { "name": "identity", "pricing": { "linear": { "base": 15, "word": 3 } } } },
"0000000000000000000000000000000000000005": {
"balance": "1",
"constructor": "6060604052341561000f57600080fd5b5b6012600102600080601160010260001916815260200190815260200160002081600019169055506022600102600080602160010260001916815260200190815260200160002081600019169055506032600102600080603160010260001916815260200190815260200160002081600019169055506042600102600080604160010260001916815260200190815260200160002081600019169055505b5b610155806100bd6000396000f30060606040526000357c0100000000000000000000000000000000000000000000000000000000900463ffffffff168063994d790a1461003e575b600080fd5b341561004957600080fd5b61008a6004808035600019169060200190919080356000191690602001909190803560001916906020019091908035600019169060200190919050506100a4565b604051808215151515815260200191505060405180910390f35b60006001800285600019161480156100c3575060026001028460001916145b156100d15760019050610121565b60006001028360001916141580156100f157506000600102826000191614155b801561011e5750816000191660008085600019166000191681526020019081526020016000205460001916145b90505b9493505050505600a165627a7a723058202082b8d8667fd397925f39785d8e804540beda0524d28af15921375145dfcc250029"
Copy link
Contributor

Choose a reason for hiding this comment

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

contract source?

use io::IoChannel;
use super::NodeFilter;

/// Contract code: https://gist.github.com/arkpar/467dbcc73cbb85b0997a7a10ffa0695f
Copy link
Contributor

Choose a reason for hiding this comment

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

ah, ok, it's here

@rphmeier rphmeier added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Aug 29, 2017
@gavofyork gavofyork merged commit d520aa2 into master Aug 29, 2017
@gavofyork gavofyork deleted the node-contract branch August 29, 2017 12:38
@arkpar arkpar added this to the 1.9 milestone Oct 13, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. B7-releasenotes 📜 Changes should be mentioned in the release notes of the next minor version release. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants