Skip to content

Minimal spec for initial consensus #10

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: eip-proposal
Choose a base branch
from
Open

Minimal spec for initial consensus #10

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
44 changes: 41 additions & 3 deletions standard.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
---
eip: <to be assigned>
title: Subscriptions
author:
author:

* Kevin Owocki <kevin@gitcoin.co> (@owocki)
* John Griffin <john@atchai.com> (@johngriffin)
* TODO - Others pls add your info here.

discussions-to: [this github issue url](https://github.com/EthereumOpenSubscriptions/standard/issues) or [Gitcoin Slack](https://gitcoin.co/slack) in #proj-subscriptions channel
Expand Down Expand Up @@ -31,7 +32,7 @@ For a dapp founder:
* since you know your subscriber numbers, churn numbers, conversion rate, you get consistent cash flow
* you get to focus on making your customers happy (as opposed to having two actors: speculators & users)

For these reasons, we think it's creating a standard way to do 'subscriptions' on Ethereum.
For these reasons, we think it's creating a standard way to do 'subscriptions' on Ethereum.


## Abstract
Expand All @@ -47,8 +48,45 @@ We also believe that creating momentum for Saas models in the Ethereum space is

## Specification

TODO
### Public Methods

#### isValidSubscription
Used to check whether a given subscription ID represents a valid subscription.

* @param \_subscriptionId The subscription ID to check
* @return a boolean to indicate whether the subscription exists and is valid

```
function isValidSubscription(bytes32 _subscriptionId)
Copy link

Choose a reason for hiding this comment

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

What is the purpose of having this as a separate function instead of just a part of a tupple that getSubscription() returns?

public
view
returns (bool success);
```

#### cancelSubscription

Called by subscriber, or by merchant in order to cancel an active subscription

* @param \_subscriptionId The subscription ID to delete
* @return a boolean to indicate whether the subscription was successfully cancelled

```
function cancelSubscription(bytes32 _subscriptionId)
Copy link

Choose a reason for hiding this comment

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

I still think it would be a good idea to consider changing this to uint256:

  • UUIDs and sha3 hashes are directly convertible to uint256
  • non-fungible token standard (721) is using unit256 for IDs, so why wouldn't we follow existing best practices?

Copy link
Contributor

@androolloyd androolloyd Aug 7, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I'm working on a model where you sign the hash of an input set of data, and provide the signature as a part of the input data set.

function getSubscriptionHash(
        address recipient,
        uint256 value,
        bytes data,
        Enum.Operation operation,
        uint256 safeTxGas,
        uint256 dataGas,
        uint256 gasPrice,
        address gasToken,
        address delegateWallet
    )
    public
    view
    returns (bytes32)
    {
        return keccak256(
            abi.encodePacked(byte(0x19), byte(0), this, recipient, value, data, operation, safeTxGas, dataGas, gasPrice, gasToken, delegateWallet)
        );
    }

returns a hash that is signed by the owner of the contract, and this signature is then stored by the merchant.

Copy link
Contributor

@androolloyd androolloyd Aug 7, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I refute the notion that because ERC-721 is currently implementing their UUID's as a uint256 that it represents "best practices", it is definitely an accepted approach.

Copy link

Choose a reason for hiding this comment

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

Probably should have used a bit different wording: if everything else is the same, why shouldn't we go with something that's already out 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 believe constructing a hash as a sum of inputs gives a deterministic ID, and that has a higher value than one that is a scalar value.

Copy link

Choose a reason for hiding this comment

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

I'm cool with using uint256, don't really see how it's different to using bytes32 since it's just a smaller conversion step unless I'm missing something.

Copy link
Contributor

@androolloyd androolloyd Aug 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

uint256 puts the onus of storing the reference to that variable off chain, rather than being able to compute it, that alone is too large of a headache IMO.

Copy link

@lknix lknix Aug 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@kermankohli yes, it's directly convertible to bytes32 (no additional cost). I was just pointing out to different types for the same thing across standards.

@androolloyd I don't follow. uint256 and bytes32 are interchangeable. Thus if you want to construct hash from a sum of inputs, that's fine - either bytes32 h or uint256(h) (conversion doesn't cost anything). Thinking more about it: having the ability to do arithmetic operations with IDs (uint256) makes more sense than index access (bytes32). I really think your choice of hash constructed IDs shouldn't be part of the standard nor should standard enforce it. There are other ways of how IDs can be constructed and it should be out of the scope of this standard.

Oh, one more thing - at some point using bytes32 instead of uint256 was more expensive (GAS-wise). Not sure if that's still the case, would like to think it got fixed.

Copy link
Contributor

Choose a reason for hiding this comment

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

@lknix ok I better understand your positioning now, to continue on like I am with bytes32 I just need to cast the uint256 in the implementation.

I know that the input gas cost is eventually to be addressed but from what I can reason it hasn't yet been implemented or isn't slated for immediate fixing.

If there is truly no cost of conversion from uint256 to bytes32 vs bytes32/uint256 gas costs, then a uint256 type would be acceptable for our purposes.

Copy link

Choose a reason for hiding this comment

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

@androolloyd yes, that was the rationale - I probably wasn't clear enough since conversions/castings usually aren't as straightforward. Cost of conversion should be 0 but let's make sure that's the case :)

public
returns (bool success);
```

### Events

#### CancelSubscription

MUST trigger on any successful call to `cancelSubscription`

```
event CancelSubscription(
bytes32 _subscriptionId,
);
```

## Rationale

Expand Down