Gitcoin Grants Proposal v1 #20
Conversation
| @param subscriptionId - A unique representation of the subscription based on the details of the agreement | ||
|
|
||
| function getSubscription( | ||
| bytes subscriptionId |
There was a problem hiding this comment.
You probably meant bytes32. Var length arrays can only be passed in internal function calls. I think it would be a good idea to consider switching to uint256 (see rationale: https://github.com/EthereumOpenSubscriptions/standard/blob/99d833792ddd4d2a422fea66fb21379c7a2ef61a/standards/stef_and_luka-standard_proposal_draft.md#subscription-identifiers)
There was a problem hiding this comment.
I did mean bytes32. If sh3 hashes are directly convertible to unit256 then it makes sense to use them. Can you point me to resources to learn more about this?
There was a problem hiding this comment.
It's a design decision, EVM works with 32 byte words (256bits)[1]. Since bytes32 and uint256 both use 2^256 bits conversion is free[2]. SO question about conversion: https://ethereum.stackexchange.com/questions/6498/how-to-convert-a-uint256-type-integer-into-a-bytes32
| @param _agent - Individual, organization, or network that performs the transaction on behalf of the subscription owner and recipient | ||
| @param _agentRewardPct - Percentage of each transaction the agent will receive as reward | ||
| @param _valuePerPeriod - Amount value approved to be exchanged per time period | ||
| @param _secondsPerTimePeriod - Seconds to elapse per time period |
There was a problem hiding this comment.
Does this support monthly subscriptions? (eg. 1st day of each month)
| address _destination, | ||
| address _recipient, | ||
| address _agent, | ||
| uint _agentRewardPct, |
There was a problem hiding this comment.
Is agent a processor (whoever is supposed to execute recurring payments)? I don't think this should be part of the standard - support for different business models (processors, fees, etc.) should come as an extension on top of the ERC948. Imo standard should support the simplest possible use case: recurring payments between a user and a provider (where provider takes care of recurring execution).
There was a problem hiding this comment.
Agent is a processor in the context of your implementation. Agent is placed here to allow for future implementations of third party processing. In this architecture, each user would be owner of their subscriptions contract. Thus, each user may choose a different agent or agent service for each subscription vs. having one processor to one provider as in your implementation. Agent may remain empty and recipient will always have the ability to pull down funds.
There was a problem hiding this comment.
I also think that processor-specific definitions are not the part of ERC-948, this can be handled externally, take a look at ERC-1228 as an example.
| address _agent, | ||
| uint _agentRewardPct, | ||
| uint _valuePerPeriod, | ||
| uint _secondsPerTimePeriod, |
There was a problem hiding this comment.
This is interesting choice, but it's impossible to set up a subscription which would be charged on the 1st of every month.
There was a problem hiding this comment.
Yes, it is impossible to have it fire on the first of each month. It is not clear to me how your implementation accomplishes this either however. I would like to learn. It appears to me that your implementation also has a rigid amount of time during each time period which is allotted on the creation of the subscription. The alternative being to supply a timestamp from an outside source. Or perhaps there is an ethereum calendar library that I am not aware of? Would you mind explaining your solution to me, or perhaps point me to resources that explain your approach? I'm happy to get on a phone call as well. Thank you.
There was a problem hiding this comment.
The basic idea is to provide billing period using a specific time unit. Time unit could be an enumerated constant (eg. month(0), day(1), hour(2),...).
So for instance, billing every week would look like: (... _timeUnit=1, _period=7 ...)
There was a problem hiding this comment.
This is the same as in our proposal. I think we should distinguish the actual payment and funds availability for the subscription.
Why not leave this up to the provider on when he wants to execute the payment, like every day, every month etc.
Using valuePerPeriod enables to define the cost of subscription and required amount of funds to be available for it without a need of implementing calendar functions (not trivial in Solidity)
There was a problem hiding this comment.
Provider can execute the payment at his own leisure that is true. However, subscriber still needs a strong guarantee that the payment won't occur before specified dates. That would require on-chain logic.
I don't think we need a complete calendar functionality with features like time zones and daylight savings... A days-per-month mapping with logic to handle leap years and some time unit transform functions would probably suffice?
There was a problem hiding this comment.
Provider can execute the payment at his own leisure that is true. However, subscriber still needs a strong guarantee that the payment won't occur before specified dates. That would require on-chain logic.
_valuePerPeriod and _secondsPerTimePeriod and current timestamp are enough to calculate the amount due in any given time. I reason about the subscription as a stream of value with given rates, esentially while time flies the amount due is increasing, moreover provider/subscriber have the gurarantee to transfer only:
The `execute` method would call `token.transfer(provider, (now - last_payment_at) * tokens_per_time_unit / time_unit)`
as we described in #16
| public | ||
| view | ||
| returns( | ||
| subscriptions[] |
There was a problem hiding this comment.
This was written as an array of structs, which cannot be done. It should be an array of id's
No description provided.