search

mastercoin-MSC / mastercore

mastercore info
mastercoin.org
MIT License
24 stars 11 forks source link

Add transaction version and type to RPC output #177

Closed dexX7 closed 9 years ago

dexX7 commented 9 years ago

Can I switch type to type_str and type_int to type?

ghost commented 9 years ago

That would be a huge mistake. Right now people are expecting the RPC frozen, so changing key fields in this function means things are more likely to break, and the way you're doing things here , by adding fields is the way to go.

I also don't see anything wrong in this pull, but I have not build tested it either.

m21 commented 9 years ago

@zathras-crypto - comments please?

dexX7 commented 9 years ago

As for the ratio a rethoical question: how would you go a head, if you wanted to identify all simple sends ever sent? This is a double question, because text based parsing won't even yield success, due to "Crowdfund Participation" != "Simple Send": Also: #123

@faizkhan00: Haha yeah, that's true. But I think there is need for a plan for this situation and I mention this especially in the context of #163 and inconsistencies within the labels. The best I came up with was to support both fields for some time and then deprechiate one. But all this is probably out of this scope at this point.

zathras-crypto commented 9 years ago

I'm supportive of adding version and type bytes to the RPC output sure.

I also agree with Faiz that we shouldn't just change RPC fields at our convenience and need to leave them static where possible. However with that said, we have been talking for some time about the need to group together and set a proper naming convention for RPC calls and attributes - the upcoming 'RPC rename' which will likely have substantial changes in it anyway.

Personal preference wise, I like 'type' being human readable and would rather keep that without a rename to typestr but that's purely preference.

Summary,

Thanks Z

dexX7 commented 9 years ago

I like 'type' being human readable ...

Question is: are RPC results for humans? I neither have a very strong opinion about this though. :)

... about the need to group together

Well, there was #161..

upcoming RPC standardization endeavour

No objections, but assuming 0.8.1 is the latest stable release, then there are already some new fields added and "it's not too late" - I'm not sure, if they are consistent, but in general there is no clean convention and a few conflicts, see #163.

RPC standardization ... group together

Just FYI: the command is parsed as string, capable of uncommon namings like:

./mastercore-cli mastercoin.transactions->send_from "mpexoDuSkGGqvqrkrjiFng38QPkJQVFyqv" ...

It's also possible and done in calls like createrawtransaction - wrapping in another JSON object, e.g.:

./mastercore-cli send '{"source":"mpexoDuSkGGqvqrkrjiFng38QPkJQVFyqv","destination": "..."}'

What I saw somewhere else: a seperation in form of different ports, e.g. RPC 8333 provides a Bitcoin interface, 8334 provides Mastercoin interface.

IIRC Bitcoin Core seems to be in favor of an upgrade to JSON-RPC 2.0, but not in the near future.

dexX7 commented 9 years ago

It would be great, if any objections or suggestions for improvement would be posted, so this can be either closed, improved or merged.

ghost commented 9 years ago

seems that these are just additions, and seem safe