OmniLayer / omnicore

OmniCore staging tree
http://www.omnilayer.org/
MIT License
756 stars 227 forks source link

BTC in crowdsales #391

Open zathras-crypto opened 8 years ago

zathras-crypto commented 8 years ago

Hey guys,

As requested I'm working on allowing BTC in crowdsales.

I noticed in the spec we have the text:

Effective with version 1 of Transaction type 51 and block #(TBD), a single crowdsale is able to accept multiple currencies, including bitcoins (currency id 0), for purchases of a Smart Property in a single crowdsale.

Two Q's:

1) The requirement at the top of roadmap is BTC in crowdsales, but the roadmap doesn't mention multiple properties in crowdsales. Are we sticking to the roadmap, or do we want to try and do multi-currency crowdsales per spec?

Note my vote is a resounding 'no' to multiple currency crowdsales, as it means we need to allow multiple crowdsales to run side by side for a single address, and I'm not really sure what the point is (though happy to be educated!).

2) What exactly does constitute a v1 crowdsale? We have plenty of valid v1 crowdsales already on-chain, and these certainly don't allow BTC or multiple currencies.

Thanks Z

zathras-crypto commented 8 years ago

3) Do BTC payments to crowdsales need to carry an Omni marker (Exodus P2PKH or OP_RETURN marker)?

I vote yes.

zathras-crypto commented 8 years ago

4) Should sender identification for BTC payments to crowdsales be based on the older "largest input by sum" logic or the newer "first vin is sender" logic?

zathras-crypto commented 8 years ago

Active BTC crowdsale:

{
    "propertyid" : 3,
    "name" : "Z_IndivisTestProperty",
    "active" : true,
    "issuer" : "mhVgSK3zaPdm752cZ7teXZaBJcMHH3ibXo",
    "propertyiddesired" : 0,
    "tokensperunit" : "10",
    "earlybonus" : 0,
    "percenttoissuer" : 0,
    "starttime" : 1466490118,
    "deadline" : 1467488310,
    "amountraised" : "0.00000000",
    "tokensissued" : "0",
    "addedissuertokens" : "0"
}

Can't participate yet as it's early days, but progress...

dexX7 commented 8 years ago

1) The requirement at the top of roadmap is BTC in crowdsales, but the roadmap doesn't mention multiple properties in crowdsales. Are we sticking to the roadmap, or do we want to try and do multi-currency crowdsales per spec?

In my opinion these are two different requirements: accepting BTC, and accepting multiple properties. While I can see the use of having multiple properties (e.g. Omni and BTC at the same time), it seems more complex, and I'd rather go step by step (i.e. only implement BTC crowdsales now).

2) What exactly does constitute a v1 crowdsale? We have plenty of valid v1 crowdsales already on-chain, and these certainly don't allow BTC or multiple currencies.

That's a mess-up from the old days (see: https://github.com/mastercoin-MSC/mastercore/issues/118). We need to use v2.

3) Do BTC payments to crowdsales need to carry an Omni marker (Exodus P2PKH or OP_RETURN marker)?

Absolutely. However, in my opinion the mechanism is very similar to BTC payments of DEx orders, so we probably should reuse that code in terms of marker selection.

4) Should sender identification for BTC payments to crowdsales be based on the older "largest input by sum" logic or the newer "first vin is sender" logic?

Assuming we reuse the marker identification from DEx payments, then we're stuck with Class B, thus "largest input by sum". Then again, I'd consider it as flaw that we can't use an empty OP_RETURN marker in these cases. Though also another point: BTC payments are very different from normal payload transactions.

zathras-crypto commented 8 years ago

Absolutely. However, in my opinion the mechanism is very similar to BTC payments of DEx orders, so we probably should reuse that code in terms of marker selection.

Hmm... I actually really disliked this model (no explicit payload describing the function) on the DEx and am sort of heading in a different direction with the BTC crowdsales stuff; requiring an explicit payload. I've always strongly favored being explicit rather than implicit about Omni Layer interactions.

For example what happens if an address has a DEx sell for BTC and a crowdsale for BTC both active, which open transaction should the BTC credit be applied against?

It'll need the "Investment Send" transaction to be brought back (renamed "Participate In Crowdsale" or similar, we should avoid nomenclature that has the term "invest" included). I realize we dropped this because participation in a crowdsale could be accomplished just with "Simple Send" but I think it's the right way to go (I also think in the DEx phase 1 revamp we should require a "Pay For Accept" payload but that's another topic).

Then again, I'd consider it as flaw that we can't use an empty OP_RETURN marker in these cases.

To be honest bud I kind of consider an empty OP_RETURN marker a flawed methodology anyway - why not use a payload with an explicit instruction such as "Pay Crowdsale" or "Pay Accept" - the cost is negligible and it removes ambiguity about the users intent (which IMO can only be a good thing).

Though also another point: BTC payments are very different from normal payload transactions.

I think that's my biggest issue - they don't need to be. Why not make BTC payments just plain Omni transactions (with payload)?. To note this is kind of what I'm playing with in BTC crowdsales at the mo, an Omni transaction "Pay Crowdsale" which carries BTC.

Thoughts?

zathras-crypto commented 8 years ago

Perhaps to simplify, instead of having multiple "Pay Crowdsale" or "Pay Accept" and so forth, we could have a single "Bitcoin Payment" payload. Something like:

Transaction Type: 80 (Bitcoin Payment) Transaction Ver: 0 TargetTxType: uint16 vOut: uint16

The vOut specifies exactly which output is considered the payment, and to whom (explicit).

The TargetTxType allows to specify what the payment is for - if the target tx type is 22 we know the payment is for an accept, if it's 51 we know the payment is for a crowdsale. This allows BTC payments in other (eg future) tx types without expressly engineering for it each time.

Just thinking out loud as I'm prototyping :)

dexX7 commented 8 years ago

It'll need the "Investment Send" transaction to be brought back (renamed "Participate In Crowdsale" or similar, we should avoid nomenclature that has the term "invest" included).

Yes, yes, yes! :)

I'd really like this. Not only for BTC crowdsales, but also for crowdsales in general and DEx payments.

Something like: ...

What if we use some other identifier, such as the hash of the crowdsale/... transaction, which even more explicitly points to the actual target, instead of TargetTxType? In this case we may even allow multiple DEx orders from the same address (not saying we necessarily should).

Just thinking out loud as I'm prototyping :)

I love the direction!

patrickdugan commented 8 years ago

I like this direction.

zathras-crypto commented 8 years ago

https://github.com/OmniLayer/omnicore/pull/399