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

Update MetaDEx network/transaction fields #67

Merged

Conversation

dexX7
Copy link
Member

@dexX7 dexX7 commented May 30, 2015

This PR removes the action byte on a transaction level.

The new transaction format:

Add:
[version=0] [type=25] [property] [number] [property] [number]

Cancel-All-At-Price:
[version=0] [type=26] [property] [number] [property] [number]

Cancel-All-Of-Currency-Pair:
[version=0] [type=27] [property] [property]

Cancel-Everything:
[version=0] [type=28] [ecosystem]

There is still some use of action bytes here and there, and until those parts are also updated, it can be completely removed, but this seems to be out of scope for now.

It resolves #47.

@zathras-crypto
Copy link

Supported, will modify my RPC branch accordingly when this is merged.

Spec will need updating.

Pinging @achamely @msgilligan @CraigSellars @marv-engine - please raise any objections before we merge this, thanks.

@zathras-crypto
Copy link

Note while I do support this 100% - this would nullify all previous MetaDEx test transactions on testnet correct?

@dexX7
Copy link
Member Author

dexX7 commented Jun 1, 2015

Yes.

Do they matter?

@zathras-crypto
Copy link

Not at all :)

@dexX7
Copy link
Member Author

dexX7 commented Jun 1, 2015

Haha well, that's good to hear. My fear was that there might be someone who already started to build something, based on the assumption there is transaction type 21 with action byte.

@zathras-crypto
Copy link

Oh there are - for example we may p*ss off a couple of folks (sorry @patrickdugan !!) who may be working on test integrations - but that doesn't mean this isn't the right thing to do.

Get it right before release :) Plus I haven't seen any objections raised on the thread discussing this proposal.

@dexX7
Copy link
Member Author

dexX7 commented Jun 1, 2015

It would be extremely helpful, if anyone who might be affected here quickly leaves a note, whether this change would be game breaking.

Although I assume changing the RPC interface has/had a greater impact, since the transaction fields are pretty low level.

@zathras-crypto
Copy link

Although I assume changing the RPC interface has/had a greater impact, since the transaction fields are pretty low level.

That's a very, very good point - since that's already been merged (the RPC changes) that should have been the "breaker-of-things" rather than underlying payload.

@achamely
Copy link

achamely commented Jun 1, 2015

The omnicore code we were working on was all based off the rpc calls so the underlying action byte change won't be a big upset. We already knew we were going to need to rework our tx21 handling all together once the rpc calls where a little more finalized/fleshed out

This commit removes the action byte on a transaction level.

The new transaction format:
```
Add:
[version=0] [type=25] [property] [number] [property] [number]

Cancel-All-At-Price:
[version=0] [type=26] [property] [number] [property] [number]

Cancel-All-Of-Currency-Pair:
[version=0] [type=27] [property] [property]

Cancel-Everything:
[version=0] [type=28] [ecosystem]
```
@dexX7 dexX7 force-pushed the oc-0.10-split-mdex-transaction-level branch from de27f02 to 3d647f5 Compare June 2, 2015 16:20
@dexX7
Copy link
Member Author

dexX7 commented Jun 2, 2015

Alright.

Please let me know, if consensus is reached.

@patrickdugan
Copy link

This may actually simplify my inventory management across many different currencies, good thinking.

@zathras-crypto
Copy link

I raised this at the meeting, consensus is to adopt this change. Feel free to merge at your leisure mate - suggest a spec change is needed to supplement this PR.

@dexX7 dexX7 merged commit 3d647f5 into OmniLayer:omnicore-0.0.10 Jun 3, 2015
dexX7 added a commit that referenced this pull request Jun 3, 2015
3d647f5 Update MetaDEx network/transaction fields (dexX7)
@marvgmail
Copy link

These new transactions look ok to me.

Is tx 21, version 0 still supported? Until some block number?

@dexX7
Copy link
Member Author

dexX7 commented Jun 9, 2015

@marv-engine: no, tx 21 doesn't exist anymore. Testnet has no block heights to enable features, and it was never live on mainnet.

If there is any application, which requires tx 21 (for whatever reason), we may provide testnet support for a limited amount of time.

Currently, the old RPC command "trade_MP" (with action byte) is still available for compability, but I think it's going to be removed rather sooner than later, too.

@dexX7 dexX7 modified the milestone: 0.0.10.0 Dec 30, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants