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

Clarified aspects of smart property creation; other cleanup #97

Merged
merged 5 commits into from
Apr 2, 2014

Conversation

marv-engine
Copy link

Reviewers (you know who you are) - please examine changes closely and let me know about any corrections.

@@ -1009,7 +1006,7 @@ The Master Protocol is at its core a layer of functionality on top of Bitcoin, u

In addition to transaction fees however there are costs associated with the outputs used to store transaction data for the various classes of transaction and these must be considered to reach a total cost to the end user for broadcasting a given Master Protocol message.

Each output must carry a value higher than the dust threshold (0.00005430 as of 6/2/14) in order for the transaction to be considered for inclusion within a block. Class B multisig outputs are significantly larger and thus command a higher minimum output value. For the purposes of this appendix default minimum values of 0.00006 and 0.00012 respectively will be used.
Each output must carry a value higher than the dust threshold (0.00005430 as of 6 Feb 2014) in order for the transaction to be considered for inclusion within a block. Class B multisig outputs are significantly larger and thus command a higher minimum output value. For the purposes of this appendix default minimum values of 0.00006 and 0.00012 respectively will be used.
Copy link
Member

Choose a reason for hiding this comment

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

Dust threshold is and was at that moment 5460 (see #50), but I think a loose phrasing is advised because there are transactions which used 5430.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I changed that on my version, but then my laptop died before I could do the pull request. This laptop can't do it.

There are a couple other changes I made. You can see all of them here: https://github.com/dacoinminster/spec/commits/master

None of them very important, but if you grab them, I won't complain :)

Copy link
Author

Choose a reason for hiding this comment

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

I overlooked that when I changed the format of the date in the text.

@ghost
Copy link

ghost commented Mar 27, 2014

Hm, LGTM, was there something specific that was controversial to change?

@dacoinminster
Copy link
Contributor

Oops! "Smart property features are unlocked as of block #290630"

That block number was for dEX, not smart property :)

@zathras-crypto
Copy link
Contributor

Yeah was just going to say that - Marv when we were talking about block start yesterday that was for DEx :)

@zathras-crypto
Copy link
Contributor

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

Wording needs clarification. Implies that if currency ID desired is 4, then ecosystem value must also be 4 (ecosystem must be same as currency ID).

Assume intention is that currency ID must be within the ecosystem selected, but on first read without prior knowledge that doesn't convey that point.

@marv-engine
Copy link
Author

That's exactly why I need proof readers. The block number insertion was a
typo. I meant to put it on DEx.

Marv Schneider
VP, User Experience/Product Usability
Engine, Inc.
marv@engine.co
240-462-6123 cell

On Thu, Mar 27, 2014 at 5:26 PM, zathras-crypto notifications@github.comwrote:

Yeah was just going to say that - Marv when we were talking about block
start yesterday that was for DEx :)

Reply to this email directly or view it on GitHubhttps://github.com//pull/97#issuecomment-38862877
.

@zathras-crypto
Copy link
Contributor

Also reasoning behind changing "Property Name" to "Property Tokens"?
The UI I've been building uses the property name so I'd like to understand if there is some issue here I'm unaware of?

@zathras-crypto
Copy link
Contributor

The sending address will receive the number of tokens calculated as the "Number Properties per unit invested" value multiplied by the number of coins (units) specified in the Simple Send message, to eight decimal places.

Does that not assume divisible shares?

@zathras-crypto
Copy link
Contributor

And that's it - sorry for all the comments :)

@marv-engine
Copy link
Author

Faiz,
I'm looking for a detailed reading to find typos, errors, things that need
clarification, etc.

Taariq,
This is for #97

Zathras,

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

Wording needs clarification. Implies that if currency ID desired is 4,
then ecosystem value must also be 4 (ecosystem must be same as currency ID).

How about:

  • 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.

Also reasoning behind changing "Property Name" to "Property Tokens"?

That's unintentional - likely overzealous find/replace. I'll undo that
change.

The sending address will receive the number of tokens calculated as the

"Number Properties per unit invested" value multiplied by the number of
coins (units) specified in the Simple Send message, to eight decimal places.

Does that not assume divisible shares?

Right you are!

How about:

For divisible properties, the sending address will receive the number of
tokens calculated as the "Number Properties per unit invested" value
multiplied by the number of coins (units) specified in the Simple Send
message, to eight decimal places. For indivisible properties, the sending
address will receive the number of tokens calculated as the "Number
Properties per unit invested" value multiplied by the number of coins
(units) specified in the Simple Send message, rounded down to an integer
number of tokens (with no fractional portion).

Thanks all! I expect there are more "gems" waiting to be found.

Marv Schneider
VP, User Experience/Product Usability
Engine, Inc.
marv@engine.co
240-462-6123 cell

On Thu, Mar 27, 2014 at 5:37 PM, zathras-crypto notifications@github.comwrote:

And that's it - sorry for all the comments :)

Reply to this email directly or view it on GitHubhttps://github.com//pull/97#issuecomment-38863971
.

@zathras-crypto
Copy link
Contributor

@marv Yep, all looks good :)

@dexX7
Copy link
Member

dexX7 commented Mar 27, 2014

Wording needs clarification. Implies that if currency ID desired is 4, then ecosystem value must also be 4 (ecosystem must be same as currency ID).

Somewhat related: #88. I asked yesterday, if there could be a GoldCoin ecosystem which was denied, so I assume there won't be an ecosystem with currency id 4?

@marv-engine
Copy link
Author

I've committed more changes to the PR to fix/clarify the items that @zathras-crypto and @dexX7 reported. As I noted in the commit, I also created issues #102 #103 #104 today that will likely result in additional detail and clarifications.

+ Valid values: 1 to 4,294,967,295 (1 = Mastercoin, 2 = Test Mastercoin, Test MSC currencies and properties have the most significant bit set)
+ Valid values: 1 to 2,147,483,647
* 1 = Mastercoin
* 2 = Test Mastercoin (Test MSC currencies and properties have the most significant bit set, so internally they have values starting with 0x80000003)
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure, if I get this right. This is a hack to get an ecosystem flag encoded inside the currency identifier field with the idea of "starting with 0 => real, starting with 1 => test"?

00000000000000000000000000000000b <> 0x00000000 <> Bitcoin (well, if we'd count it here)
00000000000000000000000000000001b <> 0x00000001 <> Mastercoin
00000000000000000000000000000010b <> 0x00000002 <> Test Mastercoin
10000000000000000000000000000000b <> 0x80000000 <> ??
10000000000000000000000000000001b <> 0x80000001 <> Test ecosystem, Mastercoin?
10000000000000000000000000000010b <> 0x80000002 <> Test ecosystem, Test Mastercoin?
10000000000000000000000000000011b <> 0x80000003 <> ??

The description seems to be in conflict with the valid values. 0x80000000 is 2,147,483,647 + 1 which would be outside of the allowed range.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, 0x80000000, 80000001, 80000002 are undefined. I don't think we'd ever
need to use them. If I did anything with them, I'd probably just map them
to 0,1,2, but I don't want to worry about that for this release.

0x80000003 would be the first property issued in the TMSC ecosystem
0x80000004 would be the second property issued in the TMSC ecosystem
3 would be the first property issued in the MSC ecosystem
4 would be the second property issued in the TMSC ecosystem

We should probably keep "1 to 4,294,967,295" as the allowable range. I
don't think implementations should drop the most significant bit for
display, because that would allow someone to create a test coin which
looked identical to a real coin (same apparent name and ID).

On Fri, Mar 28, 2014 at 9:42 AM, dexX7 notifications@github.com wrote:

In README.md:

@@ -156,7 +157,9 @@ This section defines the fields that are used to construct transaction messages.

Field: Currency identifier

  • Description: the currency used in the transaction
  • Size: 32-bit unsigned integer, 4 bytes
    -+ Valid values: 1 to 4,294,967,295 (1 = Mastercoin, 2 = Test Mastercoin, Test MSC currencies and properties have the most significant bit set)
    ++ Valid values: 1 to 2,147,483,647
    • * 1 = Mastercoin
    • * 2 = Test Mastercoin (Test MSC currencies and properties have the most significant bit set, so internally they have values starting with 0x80000003)

I'm not sure, if I get this right. This is a hack to get an ecosystem flag
encoded inside the currency identifier field with the idea of "starting
with 0 => real, starting with 1 => test"?

00000000000000000000000000000000b <> 0x00000000 <> Bitcoin (well, if we'd count it here)
00000000000000000000000000000001b <> 0x00000001 <> Mastercoin
00000000000000000000000000000010b <> 0x00000002 <> Test Mastercoin
10000000000000000000000000000000b <> 0x80000000 <> ??
10000000000000000000000000000001b <> 0x80000001 <> Test ecosystem, Mastercoin?
10000000000000000000000000000010b <> 0x80000002 <> Test ecosystem, Test Mastercoin?
10000000000000000000000000000011b <> 0x80000003 <> ??

The description seems to be in conflict with the valid values. 0x80000000
is 2,147,483,647 + 1 which would be outside of the allowed range.

Reply to this email directly or view it on GitHubhttps://github.com//pull/97/files#r11074120
.

@marv-engine
Copy link
Author

Yet another commit for PR #97 to provide more specificity and clarity. Maybe more to come.

@marv-engine
Copy link
Author

It needs to be clear that multiple replaces/appends of any sort are allowed.

@marv-engine
Copy link
Author

What needs to be done so we can merge this PR?

dacoinminster pushed a commit that referenced this pull request Apr 2, 2014
Merge now, cleanup more later if needed :)
@dacoinminster dacoinminster merged commit cb1d47f into OmniLayer:master Apr 2, 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