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

Implement LIP-9: Service Registry #223

Merged
merged 2 commits into from
Jun 8, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 45 additions & 0 deletions contracts/ServiceRegistry.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
pragma solidity ^0.4.17;

import "./ManagerProxyTarget.sol";


/**
* @title ServiceRegistry
* @dev Maintains a registry of service metadata associated with service provider addresses (transcoders/orchestrators)
*/
contract ServiceRegistry is ManagerProxyTarget {
// Store service metadata
struct Record {
Copy link
Member

Choose a reason for hiding this comment

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

If we changed the Record struct in the future, the storage layout will be screwy right? If that's the case, I prefer to have the mapping from address to string, that way this potential mistake doesn't happen.

Copy link
Member Author

Choose a reason for hiding this comment

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

Adding additional fields to the Record struct in an upgrade is safe as long as 1) we do not remove/replace existing fields 2) the Record struct is not used in an array (it is only used in a mapping right now)

string serviceURI; // Service URI endpoint that can be used to send off-chain requests
}

// Track records for addresses
mapping (address => Record) private records;

// Event fired when a caller updates its service URI endpoint
event ServiceURIUpdate(address indexed addr, string serviceURI);

/**
* @dev ServiceRegistry constructor. Only invokes constructor of base Manager contract with provided Controller address
* @param _controller Address of a Controller that this contract will be registered with
*/
function ServiceRegistry(address _controller) public Manager(_controller) {}

/**
* @dev Stores service URI endpoint for the caller that can be used to send requests to the caller off-chain
* @param _serviceURI Service URI endpoint for the caller
*/
function setServiceURI(string _serviceURI) external {
Copy link

Choose a reason for hiding this comment

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

Are there any potential negative externalities for the rest of the network if a bunch of non-transcoders set their service URI records here?

Copy link
Member Author

@yondonfu yondonfu Jun 5, 2018

Choose a reason for hiding this comment

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

Hm since a broadcaster client would only care about the service URI record for the transcoder address that was assigned to the broadcaster's transcode job if non-transcoder users set their own service URI record they would pay the tx cost, but at least in the current proposed networking architecture those records will never be used in practice unless the non-transcoder user becomes a transcoder in the future. Perhaps something to be wary about though is if a transcoder's key is compromised such that an attacker can change the service URI record right before a broadcaster looks it up to send requests to the transcoder

Copy link

Choose a reason for hiding this comment

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

Yeah, keeping the transcoder's keys keys cold would entail something like the discussion in livepeer/LIPs#7 which probably should be done separately from this change.

I was mostly wondering if filling the record mapping with unneeded entries could lead to higher costs for the network down the road, eg during a migration or with any enactment of 'data rents', but that's all fairly speculative. LGTM.

records[msg.sender].serviceURI = _serviceURI;

ServiceURIUpdate(msg.sender, _serviceURI);
}

/**
* @dev Returns service URI endpoint stored for a given address
* @param _addr Address for which a service URI endpoint is desired
*/
function getServiceURI(address _addr) public view returns (string) {
return records[_addr].serviceURI;
}
}
3 changes: 3 additions & 0 deletions migrations/3_deploy_contracts.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ const ContractDeployer = require("../utils/contractDeployer")

const Controller = artifacts.require("Controller")
const Minter = artifacts.require("Minter")
const ServiceRegistry = artifacts.require("ServiceRegistry")
const BondingManager = artifacts.require("BondingManager")
const JobsManager = artifacts.require("JobsManager")
const RoundsManager = artifacts.require("RoundsManager")
Expand Down Expand Up @@ -38,6 +39,8 @@ module.exports = function(deployer, network) {
roundsManager = await lpDeployer.deployProxyAndRegister(RoundsManager, "RoundsManager", controller.address)
}

await lpDeployer.deployProxyAndRegister(ServiceRegistry, "ServiceRegistry", controller.address)

deployer.logger.log("Initializing contracts...")

// Set BondingManager parameters
Expand Down
19 changes: 19 additions & 0 deletions test/integration/JobAssignment.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import {createTranscodingOptions} from "../../utils/videoProfile"
import BigNumber from "bignumber.js"

const Controller = artifacts.require("Controller")
const ServiceRegistry = artifacts.require("ServiceRegistry")
const BondingManager = artifacts.require("BondingManager")
const JobsManager = artifacts.require("JobsManager")
const AdjustableRoundsManager = artifacts.require("AdjustableRoundsManager")
Expand All @@ -12,6 +13,7 @@ contract("JobAssignment", accounts => {
const TOKEN_UNIT = 10 ** 18

let controller
let registry
let bondingManager
let jobsManager
let roundsManager
Expand All @@ -33,6 +35,9 @@ contract("JobAssignment", accounts => {
controller = await Controller.deployed()
await controller.unpause()

const registryAddr = await controller.getContract(contractId("ServiceRegistry"))
registry = await ServiceRegistry.at(registryAddr)

const bondingManagerAddr = await controller.getContract(contractId("BondingManager"))
bondingManager = await BondingManager.at(bondingManagerAddr)

Expand All @@ -59,16 +64,22 @@ contract("JobAssignment", accounts => {
await token.approve(bondingManager.address, 50000, {from: transcoder1})
await bondingManager.bond(50000, transcoder1, {from: transcoder1})
await bondingManager.transcoder(10, 5, 5, {from: transcoder1})
// Set service URI for transcoder 1
await registry.setServiceURI("transcoder1URI", {from: transcoder1})

// Register and bond transcoder 2 with 30% of the total active stake
await token.approve(bondingManager.address, 30000, {from: transcoder2})
await bondingManager.bond(30000, transcoder2, {from: transcoder2})
await bondingManager.transcoder(10, 5, 1, {from: transcoder2})
// Set service URI for transcoder 2
await registry.setServiceURI("transcoder2URI", {from: transcoder2})

// Register and bond transcoder 3 with 20% of the total active stake
await token.approve(bondingManager.address, 20000, {from: transcoder3})
await bondingManager.bond(20000, transcoder3, {from: transcoder3})
await bondingManager.transcoder(10, 5, 1, {from: transcoder3})
// Set service URI for transcoder 3
await registry.setServiceURI("transcoder3URI", {from: transcoder3})

// Initialize new round and initialize new active transcoder set
await roundsManager.mineBlocks(roundLength.toNumber())
Expand All @@ -94,6 +105,7 @@ contract("JobAssignment", accounts => {
let jobCreationRound
let rand
let electedTranscoder
let expServiceURI

let jobsCreated = 0

Expand All @@ -111,19 +123,26 @@ contract("JobAssignment", accounts => {

switch (electedTranscoder) {
case transcoder1:
expServiceURI = "transcoder1URI"
transcoder1JobCount++
break
case transcoder2:
expServiceURI = "transcoder2URI"
transcoder2JobCount++
break
case transcoder3:
expServiceURI = "transcoder3URI"
transcoder3JobCount++
break
default:
expServiceURI = ""
nullAddressJobCount++
break
}

// Check for correct service URI
assert.equal(await registry.getServiceURI(electedTranscoder), expServiceURI, "wrong service URI")

await roundsManager.mineBlocks(1)

if (!(await roundsManager.currentRoundInitialized())) {
Expand Down
75 changes: 75 additions & 0 deletions test/unit/ServiceRegistry.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
import Fixture from "./helpers/Fixture"

const ServiceRegistry = artifacts.require("ServiceRegistry")

contract("ServiceRegistry", accounts => {
describe("constructor", () => {
it("invokes base Manager contract constructor", async () => {
// Use dummy Controller
const controller = accounts[0]
const registry = await ServiceRegistry.new(controller)

assert.equal(await registry.controller.call(), controller, "wrong Controller address")
})
})

let fixture
let registry

before(async () => {
fixture = new Fixture(web3)

// Use dummy Controller in these unit tests
// We are testing the logic of ServiceRegistry directly so we do not
// interact with the contract via a proxy
// Thus, we do not need an actual Controller for the tests
const controller = accounts[0]

registry = await ServiceRegistry.new(controller)
})

beforeEach(async () => {
await fixture.setUp()
})

afterEach(async () => {
await fixture.tearDown()
})

describe("setServiceURI", () => {
it("stores service URI endpoint for caller", async () => {
await registry.setServiceURI("foo", {from: accounts[0]})
await registry.setServiceURI("bar", {from: accounts[1]})

assert.equal(await registry.getServiceURI(accounts[0]), "foo", "wrong service URI stored for caller 1")
assert.equal(await registry.getServiceURI(accounts[1]), "bar", "wrong service URI stored for caller 2")
})

it("fires ServiceURIUpdate event", async () => {
let e = registry.ServiceURIUpdate({})

e.watch(async (err, result) => {
e.stopWatching()

assert.equal(result.args.addr, accounts[0], "wrong address in ServiceURIUpdate event")
assert.equal(result.args.serviceURI, "foo", "wrong service URI in ServiceURIUpdate event")
})

await registry.setServiceURI("foo", {from: accounts[0]})
})
})

describe("getServiceURI", () => {
it("returns service URI endpoint for provided address", async () => {
await registry.setServiceURI("foo", {from: accounts[0]})
await registry.setServiceURI("bar", {from: accounts[1]})

assert.equal(await registry.getServiceURI(accounts[0]), "foo", "wrong service URI stored for caller 1")
assert.equal(await registry.getServiceURI(accounts[1]), "bar", "wrong service URI stored for caller 2")
})

it("returns empty string for address without stored service URI endpoint", async () => {
assert.equal(await registry.getServiceURI(accounts[5]), "", "should return empty string for address without service URI")
})
})
})