Add collateralization & examples #1
Conversation
Theodus
force-pushed
the
theodus/collateralization
branch
4 times, most recently
from
b374cab to
e5a103a
Compare
Theodus
force-pushed
the
theodus/collateralization
branch
from
e5a103a to
5994d51
Compare
src/examples/DataService.sol
Outdated
| /// Add provider and fund their future payment. | ||
| function addProvider(address _provider, uint128 _payment) public onlyOwner { | ||
| require(_payment > 0); | ||
| require(providers[_provider].payment == 0, "provider exists"); | ||
| providers[_provider] = ProviderState({deposit: 0, payment: _payment}); | ||
| collateralization.token().transferFrom(msg.sender, address(this), _payment); |
There was a problem hiding this comment.
Can you elaborate what this is/does and why it's onlyOwner?
src/Collateralization.sol
Outdated
| /// @param _unlock Unlock timestamp of the new deposit, in seconds. Set to a nonzero value to lock deposit. | ||
| /// @return id Unique ID associated with the new deposit. | ||
| function deposit(address _arbiter, uint256 _value, uint64 _unlock) public returns (uint128) { | ||
| lastID += 1; |
There was a problem hiding this comment.
I'm thinking this way to create IDs is unpredictable, since the resulting ID can be larger than expected if someone creates a deposit in the same block. So it might be a good idea to instead use a per-sender predictable ID e.g. keccak256(msg.sender, nonces[msg.sender]) so that a caller can batch a deposit + lock or getDeposit or some other action knowing what the ID will be.
There was a problem hiding this comment.
And a depositFrom function could be useful for users to create deposits on behalf of other depositors (as long as they have a token approval)
Edit: actually, I think the opposite may be more useful: a depositFor or depositTo where a third party starts a deposit on behalf of someone else, providing the tokens. We have a stakeTo in the Staking contract that serves this purpose. This could be useful, among other things, if a service provider has tokens on several accounts but uses a single address to provide services.
There was a problem hiding this comment.
Echoing Pablo's comments on IDs, I would also add block.chainId to the id generation/derivation.
I can't think of any potential problems now, but since we will be building on top of this I think it's better to add it and rule out any silly cross-chain replay attacks.
There was a problem hiding this comment.
aaah good point, making deposit IDs unique cross-chain could prove useful in the future
src/Collateralization.sol
Outdated
| /// This contract manages Deposits as described above. | ||
| contract Collateralization { | ||
| event Deposit(uint128 indexed id, address indexed arbiter, uint256 value, uint64 unlock); | ||
| event Lock(uint128 indexed id, uint64 unlock); |
There was a problem hiding this comment.
nit/suggestion: Event names could be a bit more verbose, e.g. DepositCreated, DepositLocked, etc.
Theodus
force-pushed
the
theodus/collateralization
branch
from
7c72228 to
033ab68
Compare
This PR adds the core collateralization contracts and some examples for lending, loan aggregation, and data services.