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

Tx51 (crowdsale) version 1 accepts multiple currencies, including bitcoins #185

Merged
merged 7 commits into from
Jun 12, 2014

Conversation

marv-engine
Copy link

We need to specify the block number where tx51 version 1 will be turned on.

  • tx51 version 1 allows 0 (bitcoin) for Currency Identifier desired
  • early bird bonus no longer tied to weeks (this change has been undone by a later commit that's part of this PR)
  • number of tokens issued limited by max Number of Coins value
  • Number of Coins now a signed 8-byte integer

We need to specify the block number where tx51 version 1 will be turned on.

* tx51 version 1 allows 0 (bitcoin) for Currency Identifier desired
* early bird bonus no longer tied to weeks
* number of tokens issued limited by max Number of Coins value
* Number of Coins now a signed 8-byte integer

percentage = (("Deadline" value in seconds - transaction timestamp in seconds) / 604800) * "Early bird bonus %/week" value
(1 + (percentage / 100.)) * "Number Properties per Unit Invested" value * the number of coins sent by the purchaser
Copy link
Member

Choose a reason for hiding this comment

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

"100."? What's the point used for there? If it is, what I think, then I suggest to use "100.0", "100f" or similar instead. It's not all that clear, if this is simply a typo or intended.

@dexX7
Copy link
Member

dexX7 commented Jun 3, 2014

Just to make sure: the way the bonus is calculated is fundamentally different now? Say for example I wanted to create a crowdsale with a bonus structure similar to the Exodus funding.

In version 0 I'd set the deadline to four weeks in the future and the bonus to 10 %.
In version 1 I'd set the deadline to four weeks in the future, but the bonus to 40 %.

Correct?

@Bitoy
Copy link

Bitoy commented Jun 3, 2014

@dexX7 you are correct regarding calculation of bonus.

@marv-engine
Copy link
Author

@dexX7 I can change 100. to 100.0

My goal is for the implementations to use enough precision (double precision?) for all calculations and then convert to the target data type after the calculation is complete.

The reason to remove any reference to weeks for the early bird bonus is that it was confusing the way it was written. People thought the bonus declined as a weekly step function, not linearly by the second as it does. Also, the week reference was unclear about how partial weeks are handled (e.g. 30 days vs 28). Version 1 is intended to eliminate the confusion and make it easy to use.

@dexX7
Copy link
Member

dexX7 commented Jun 3, 2014

Yup, agreed. This way it's more intuitive.

I think we should define percision spec wide - this is also relevant for Dev MSC and DEx. Double percision should be fine.

@ghost
Copy link

ghost commented Jun 4, 2014

Is there any worry that using double precision floats will lead to floating point arithmetic issues?

@dacoinminster
Copy link
Contributor

I've looked through these changes. Still needs updates due to recent breakthough regarding TX51. (multiple currencies going at once, or updates to exchange rates). I assume that's still coming (unless maybe I missed it)

@marv-engine
Copy link
Author

@dacoinminster yes. i was intending to do that PR after this one, but I can combine them if you want.

@dacoinminster
Copy link
Contributor

Either way is fine.

@marv-engine
Copy link
Author

ok, i'll add it to this one.

@marv-engine
Copy link
Author

Re: enabling a crowdsale to accept multiple currencies by using multiple tx51's -
the spec currently says:

MSC address may have only one crowdsale active per ecosystem at any given time, eliminating the need for participants to specify which crowdsale from that address they are participating in when they purchase. See Participating in a crowdsale below.

That sentence is insufficient because an address could have 2 active crowdsales - 1 in each ecosystem - with the same currency id desired. The protocol wouldn't know which crowdsale a purchaser is participating in. I doubt that anyone will initiate cross-ecosystem crowdsales, but the protocol has to handle it if it's allowed.

I see two ways to address this:

  1. the currency id desired has to be in the same ecosystem as the new currency id (not a bad idea)
    • this doesn't address the case where both crowdsales accept bitcoins
  2. if a currency id is already specified for an active crowdsale, that currency id can't be used for a concurrent crowdsale in the other ecosystem, or
  3. combine them:
    • the currency id desired has to be in the same ecosystem as the new currency id
      • doesn't apply when bitcon is the desired currency
    • if bitcoin is already specified as the currency id for an active crowdsale, then bitcoin can't be used for a concurrent crowdsale in the other ecosystem,

So, any preference or objection to item 2 or 3? I like 3 because it's an additional way to keep the ecosystems from cross-pollenating.

@dacoinminster
Copy link
Contributor

Um, just prevent two crowdsales from the same address even in different ecosystems. Much simpler

@marv-engine
Copy link
Author

Easy enough - just remove "per ecosystem" from the existing sentence.

This change will apply to version 1 of tx51. Is it safe to assume that no one has tried, or will try, to exercise this case with version 0 of tx51? The spec didn't address this case. I don't know if any client detected this case, and if so, what the code did.

@marv-engine
Copy link
Author

When accepting multiple currencies, can the originating tx51 be updated to zero for the Number Properties per Unit Invested value? I think the issuer has to be able to stop accepting any of the currencies, including the original one.

So, subsequent tx51's can stop accepting currencies as long as there's still one currency being accepted? Or, can an issuer effectively suspend temporarily all participation by setting Number Properties per Unit Invested value to zero for each of the currencies he's accepting?

@dacoinminster
Copy link
Contributor

Yes, they can set the original one to zero. Yes, they can set all of them
to zero if they want.

If the new TX51 has a different end date, premine, early bird bonus, name,
URL, etc, those differences are IGNORED since those fields cannot be
changed during the crowd sale. Note Ron suggested making them invalid, but
I think it's simpler to just not look at those fields after the first TX51.

On Thu, Jun 5, 2014 at 3:43 PM, Marv Schneider notifications@github.com
wrote:

When accepting multiple currencies, can the originating tx51 be updated to
zero for the Number Properties per Unit Invested value? I think the issuer
has to be able to stop accepting any of the currencies, including the
original one.

So, subsequent tx51's can stop accepting currencies as long as there's
still one currency being accepted? Or, can an issuer effectively suspend
temporarily all participation by setting Number Properties per Unit
Invested value to zero for each of the currencies he's accepting?


Reply to this email directly or view it on GitHub
#185 (comment).

@marv-engine
Copy link
Author

If the new TX51 has a different end date, premine, early bird bonus, name,
URL, etc, those differences are IGNORED since those fields cannot be
changed during the crowd sale. Note Ron suggested making them invalid, but
I think it's simpler to just not look at those fields after the first TX51.

I understood that the same end date (Deadline) is what indicates that a new tx51 applies to the active crowdsale (and either adds a desired currency or modifies the terms for an existing one), and a different end date indicates an attempt to create a concurrent crowdsale - which is not allowed. I agree that the fields other than Currency Identifier Desired and Number Properties per Unit Invested are ignored for tx51's after the original one for an active crowdsale.

We still need to address my question a few comments back in #185 (comment)

@Bitoy
Copy link

Bitoy commented Jun 6, 2014

Good move on allowing the issuer to accept multiple currency for crowd funding. I'm not sure how it works but here is my input.

A new tx 51 that is sent before the crowd funding is closed or has ended means it is a "Add new currency for current crowd funding". (If it is sent after a tx 51 is closed or ended, it is a new SP)

Ex.
Alice send the first tx51. It defines the smart property.
Currency Identifier Desired = 0 (Bitcoins)
Number Properties per Unit Invested =17000

Alice sends a second tx51
Currency Identifier Desired = 1 (MSC)
Number Properties per Unit Invested =3400

Alice sends a third tx51
Currency Identifier Desired = 3 (MAIDSAFE)
Number Properties per Unit Invested =100

Creates a crowd funding that accepts Bitcoins, MSC, or MAIDSAFE.

If the "Currency Identifier Desired" already exist in the previous tx51s, the tx is invalid.
ex.
Alice sends a fourth tx51
Currency Identifier Desired = 2 (MSC)
Number Properties per Unit Invested =1100

invalid because MSC is already defined in Second tx51. (This means an Issuer has only one chance to set the Number Properties per coin )

Marv Schneider added 2 commits June 6, 2014 11:10
tweaked the wording to account for a crowdsale accepting multiple currencies.
@dexX7
Copy link
Member

dexX7 commented Jun 6, 2014

I haven't reviewed the new version of this, but @Bitoy: the option to edit the number of properties credited for token X on the fly is there to adjust to changing exchange rates.

@marv-engine
Copy link
Author

@Bitoy, @dexX7 Yes, the number of tokens issued per unit sent can be updated multiple times during the crowdsale on a currency-by-currency basis. If it's set to 0, then the crowdsale is not accepting that currency, either temporarily or permanently. This applies to any currency accepted by the crowdsale, including the original one.

It's up to the issuer to prep for/deal with participant reaction to changes in the issue rate.

@marv-engine marv-engine changed the title Tx51 (crowdsale) version 1 accepts bitcoins Tx51 (crowdsale) version 1 accepts multiple currencies, including bitcoins Jun 6, 2014
@dexX7
Copy link
Member

dexX7 commented Jun 6, 2014

General rant ;).. inconsistent usage of case sensitivity (we should really push a general document related to address terminology, but given that I'm no native English speaker, I don't feel appropriate for this role). "tx51" may be replaced by the full identifier.

Also slightly related:
#188 Revision control, start to tag spec releases (...)

@dexX7
Copy link
Member

dexX7 commented Jun 6, 2014

From another user:

How are crowdsale funding transactions handled that occur in the same block as the TX51 transaction which changes the properties, do those transactions use the old properties or the new ones?

I assume this is done as usual? See: #131

Edit:

It was suggested to explicitly include an exchange rate in the payment transaction for the case that an user sends coins/tokens and the crowdsale terms changed in the meantime.

@dacoinminster
Copy link
Contributor

Sure - they can suspend all if they want

@Bitoy
Copy link

Bitoy commented Jun 7, 2014

@marv-engine ok I now understand why Updating the no. Of properties per coin is left to the issuer. Crowd sale v 1 looks ready. ( Can't wait to see it in action :)

@marv-engine
Copy link
Author

This issue still needs to be addressed:

... (In version 0 of tx51) an address could have 2 active crowdsales - 1 in each ecosystem - with the same currency id desired. The protocol wouldn't know which crowdsale a purchaser is participating in.

So,

Is it safe to assume that no one has tried, or will try, to exercise this case with version 0 of tx51? The spec didn't address this case. I don't know if any client detected this case, and if so, what the code did.

How do we prevent this spec bug from being exercised by a user?

@marv-engine
Copy link
Author

In response to the most recent comment from @dexX7 #185 (comment)

My expectation, based on #131 - A tx51 that changes a crowdsale affects any sends that come after that tx51, regardless of which block(s) the sends are in.

It was suggested to explicitly include an exchange rate in the payment transaction for the case that an user sends coins/tokens and the crowdsale terms changed in the meantime.

Simple Send and bitcoin Send messages don't have a way to include an exchange rate. (I do have some ideas that should work for Simple Send without manual intervention, but wouldn't work for bitcoin Send.)

It's left to the issuer to respond to requests for some kind of adjustment if the purchaser doesn't like the exchange rate he ended up with or if the crowdsale closed before the payment was processed. I wouldn't want to be either party in a case involving a large payment.

@dexX7
Copy link
Member

dexX7 commented Jun 7, 2014

Simple sends could be enhanced by additional data (exchange rates) and something simliar could be attached to Bitcoin payments -- but I really don't like it. I agree, this is a topic which issuers should address. In most cases I assume the issuer likes to act proactive anyway.

Edit: there could be a grace periode. For example a change takes effect three blocks after the first confirmation. Same for closing a crowdsale. I suggested this at some other place earlier (#132) and I don't really like it either. I'd rather see, where this is going and if it evolves to be a problem.

@Bitoy
Copy link

Bitoy commented Jun 8, 2014

@marv-engine
For v0 currency desired can't be 0.
For v1 Bitcoins must be defined as for use only in ecosystem 1
That way when a user sends bitcoins it is for the crowd sale of ecosystem 1

Assuming the user creates 2 crowd sales.
In Ecosystem 1 you can only accept "msc coins" derived coins and bitcoins.
In ecosystem 2 (test msc) you can only accept "test msc coins" .

User that send " test msc" coins goes to crowd sale Eco 2
User that sends msc coins or bitcoins it is a crowd sale Eco 1

There is no conflict.

@dexX7
Copy link
Member

dexX7 commented Jun 8, 2014

But Test MSC can be sold for BTC?

@Bitoy
Copy link

Bitoy commented Jun 8, 2014

Ah ok thanks @dexX7. The dex tx 21 doesn't have an ecosystem property so it's valid to sell test msc for bitcoins. ( just trying to fix the conflict :)

@dexX7
Copy link
Member

dexX7 commented Jun 8, 2014

The ecosystem flag does nothing else than creating trouble.. ;) :P

@Bitoy: tx 20 (not 21!*) uses currency ids which imply the ecosystem. TMSC are a special case related to the id and ecosystem. See: Field: Currency identifier

So in a strict interpretation: MSC is ecosystem 1, TMSC is ecosystem 2. Therefore, based on tx 20, Bitcoin trades are allowed in both ecosystems.

@marv-engine:

A MSC address may have only one crowdsale active per ecosystem at any given time, eliminating the need for participants to specify which crowdsale from that address they are participating in when they purchase.

I didn't see it at first, but there is acutally another restriction. It's not attached to the transaction and only loosely written in the general description for smart properties:

The Ecosystem for the property must be the same as the ecosystem for the "Currency identifier desired", i.e. both must be in the Mastercoin ecosystem or both must be in the Test Mastercoin ecosystem.

Tx 51 is related to the creation of a crowdsale and if we decide to limit version 1 to allow only one crowdsale overall, then the conflict related for Bitcoin seems to be solved, because even if a user goes ahead and creates a Bitcoin nominated crowdsale in whatever ecosystem with tx51v1 and then uses tx51v0 to add another one, the second one must be in a different ecosystem (as required by v0) and since v0 can't use Bitcoin, there is no case where a user might create two crowdsales with Bitcoin?

Edit: if the limitation of one active crowdsale in v1 is applied, this also implies changing a crowdsale via tx51v1 can only be used, if there is only one, and not two, active crowdsales.

*How does tx 21 (sell token for token) address trades between ecosystems? Something that may be considered as restriction is also written in the general "smart property" description:

Test Mastercoin properties have the most significant bit set to distinguish them from real properties, and they cannot be traded against real Mastercoins nor otherwise interact with non-test properties

Is this applied? I strongly suggest we don't set rules outside of the direct scope of transactions - statements like this have no version number.

@dacoinminster
Copy link
Contributor

Guys, no exchange rates in simple sends. Too complicated. This would become an "investment send". Which is something we can always add later

It's up to the issuer to fix any problems caused by their monkeying with exchange rates!

@marv-engine
Copy link
Author

@dacoinminster we're not looking to add exchange rates to simple sends. But, please look at my #185 (comment)

@marv-engine
Copy link
Author

from a conversation yesterday, @dacoinminster says to disable tx51 version 0 starting with the same block that version 1 is enabled.

@dexX7
Copy link
Member

dexX7 commented Jun 11, 2014

Fine. I hope there will some kind of announcement? FYI: Tx20v0 was nerver disabled.

So only one active crowdsale, yes? Should I open a new issue to address inter-ecosystem trading via tx21?

@marv-engine
Copy link
Author

@dexX7 as you noted in an earlier comment, there is no exposure with tx21v0, so technically we don't need to disable it. Thanks for that catch, from the spec:

The Ecosystem for the property must be the same as the ecosystem for the "Currency identifier desired", i.e. both must be in the Mastercoin ecosystem or both must be in the Test Mastercoin ecosystem.

Yes, please create another issue to discuss tx21 (sell one MSC-based currency for another MSC-base currency). This PR is about tx51 (crowdsale).

dacoinminster pushed a commit that referenced this pull request Jun 12, 2014
Tx51 (crowdsale) version 1 accepts multiple currencies, including bitcoins
@dacoinminster dacoinminster merged commit 8d59658 into OmniLayer:master Jun 12, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants