Comments (9)
change the order of transaction fields
Seems (arguably) change with quite limited value proposition.
Please discuss, whether transaction type 21 can be restructured, such that the ecosystem field is no longer at the end of the data package, and the non-validated fields are removed
What do we gain here in tangible terms - I think smaller payloads is a worthwhile sentiment but when all current MetaDEx functionality fits into Class C already, what tangible benefits do we realize other than shaving a few bytes (maybe 3-5%?) from an average 300 byte transaction? We slimmed down so much with Class C already that I wonder how much we gain from this.
or ideally, if the transaction type can be replaced and split into 4 seperated ones (for each action) without action byte.
I actually kind of support this because I think it makes the whole thing a little cleaner and less ambiguous but I'm not sure - would need to spend a little more time looking over your suggested changes (from a brief look I saw you were still using the action byte in the RPC calls, which doesn't make sense to me if we're removing it?).
TL:DR; I guess my view comes down to that of:
"How much delay and how much risk does this introduce, and for what gains?"
If the answer to delay and risk is minimal/low and we get good gains from it I'm supportive - but I also know leadership won't allow us to push back release for long.
Also just want to say excellent stuff & don't take my feedback as negative :)
from omnicore.
Thanks @zathras-crypto!
In my opinion it should never had been specified this way, and it already caused trouble, because empty/zero fields were initially rejected.
The actual gain is probably small, but since it's not yet live, and the scope of the changes isn't huge, I think it's better to optimize now, rather than adding some v1 in the future.
I saw you were still using the action byte in the RPC calls, which doesn't make sense to me if we're removing it?
The interface of trade_MP
was unchanged for the sake of compability, because all RPC tests I wanted to run also use trade_MP
with action byte, but it creates the different payloads/transactions tx 25: new offer
, tx 26: cancel at price
, tx 27: cancel pair
, tx 28: cancel everything
as expected.
The changes very roughly:
MSC_TYPE_METADEX
was removed and replaced byMSC_TYPE_MDEX_NEW
,MSC_TYPE_MDEX_PRICE
,MSC_TYPE_MDEX_PAIR
,MSC_TYPE_MDEX_ECOSYSTEM
- any positive check (RPC, UI) for
MSC_TYPE_METADEX
was replaced by"is either MSC_TYPE_MDEX_NEW, _PRICE, _PAIR, _ECOSYSTEM?"
* - any negative negative check for
MSC_TYPE_METADEX
was replaced by"is none of MSC_TYPE_MDEX_NEW, _PRICE, _PAIR, _ECOSYSTEM?"
* logicMath_MetaDEx()
was split intologicMath_MetaDEx_New()
,logicMath_MetaDEx_CancelPrice()
,logicMath_MetaDEx_CancelPair()
,logicMath_MetaDEx_CancelEcosystem()
- the minimum accepted payload size was reduced from 8 to 5 byte
CreatePayload_MetaDExTrade()
was removed, andCreatePayload_MetaDExTrade()
,CreatePayload_MetaDExCancelPrice()
,CreatePayload_MetaDExCancelPair()
,CreatePayload_MetaDExCancelEcosystem()
were added to create the different payloadstrade_MP/trade_OMNI
was adjusted such that the correct payload is created- a similar adjustment was used for the UI (create trade dialog, cancel trades dialog)
*Example (sort of lame, because there are no pending objects for cancel orders at this point, but the intention was to provide exactly the same behavior as before):
- if (p_pending->type == 21) { htxo.txType = "MetaDEx Trade"; htxo.fundsMoved = false; } // send and metadex trades are the only supported outbound txs (thus only possible pending) for now
+ if (MSC_TYPE_MDEX_NEW == p_pending->type || MSC_TYPE_MDEX_CANCEL_PRICE == p_pending->type || MSC_TYPE_MDEX_CANCEL_PAIR == p_pending->type || MSC_TYPE_MDEX_CANCEL_ECOSYSTEM == p_pending->type) { htxo.txType = "MetaDEx Trade"; htxo.fundsMoved = false; } // send and metadex trades are the only supported outbound txs (thus only possible pending) for now
The actual logic, the GUI etc. already differentiates between new or cancel orders for obvious reasons, and unless I missed something, this it mostly all that's to do. :)
from omnicore.
The actual gain is probably small, but since it's not yet live, and the scope of the changes isn't huge, I think it's better to optimize now, rather than adding some v1 in the future.
Understood - and that's kind of my feeling about the RPC layer too - why bother trying to maintain backwards compatibility with something that's never been released when instead we can just start fresh with dedicated RPC calls (sendtrade_OMNI, sendcanceltradebypair_OMNI sendcanceltradebyprice_OMNI, sendcancelalltrades_OMNI) or something to that effect - this way if we are saying we want to remove ambiguity let's do it across the board.
I realize it's a PITA as you'd have to redo the tests (and anyone who's playing with this stuff in test integrations would have to adapt but I don't think there are more than a couple of projects in that basket).
from omnicore.
sendtrade_OMNI, sendcanceltradebypair_OMNI sendcanceltradebyprice_OMNI, sendcancelalltrades_OMNI
Imho this is something we should adopt, even without a new transaction format.
I realize it's a PITA as you'd have to redo the tests
Haha, this is really not an issue. ;) Most of the meta DEx tests are currently only available via the old Python tests, and still need to be added to OmniJ anyway.
from omnicore.
Omniwallet/Omniengine had limited support for original metadex development and that was only on testnet. There was going to need to be a major update/rewrite of that portion of the code with the new metadex launch anyway. I'm all for removing ambiguity and getting very specific/defined actions calls so the spit definitions of tx 25/26/27/28 is good in my book and should save some headache down the line when others start looking into integration
from omnicore.
Thanks for the feedback, @achamely! Ideally Omniwallet should use native Omni Core commands, and if something is missing, please feel free to open a ticket.
To tackle the topic, I suggest to move forward with Seperate RPC commands for DEx actions #48, and pin down all areas where action bytes are involved. Almost all parts already have a seperate logic, where the action byte usually acts as switch point. As last step there could be a seperation on a transaction level, if we decide this is the way to go.
from omnicore.
@dexX7 once native commands for creating tx's from unknown address is available we'll use it. But at the moment OmniCore needs to have the address added/scanned before it will create tx's. Unfortunately for Omniwallet, this isn't possible. 1: too many addresses, 2: we don't have users private keys to add.
So we'd need support for the addrindex functionality to create tx's or get btc balance info
from omnicore.
Interesting to note, even after a --startclean
old trades still show up (albeit 'cancelled'). I'm looking into why now...
from omnicore.
Scratch above comment, looks like I made a mess of the compile :( d'oh
from omnicore.
Related Issues (20)
- What happens to tokens sent to a M address non generated by an Omnilite Core wallet? HOT 2
- Does anyone know that bitcoin core and Omni core can be installed together HOT 2
- I have "invalid transaction" in omnicore explorer but the transaction was proceed in the BTC network as normal transaction ...what do i need to do ? HOT 2
- one big risk that tx result from omnicore rpc is different from omniexplorer.info HOT 9
- Synchronization is too slow. I want normal peer information HOT 2
- Bugs n alice
- omni_gettradehistoryforaddress : Add `blocktime` for each match in matches array of JSON response HOT 1
- Ether
- V0.12.0 release is mandatory but provides no estimation or date for activiation HOT 1
- JSON-RPC method to find transactions in initial OMNI (MSC) crowdsale HOT 2
- Sendtomany transaction in omni_listtransactions & omni_listpendingtransactions
- Update Copyright year to 2022 (Omni Core 0.12.0 release)
- RegTest failed when running against omnicore-qt v0.12.0 HOT 1
- Document Gitian (reproducible) build procedure for Omni Core HOT 1
- Running Omni node in prune mode
- Performance improvements
- Lost 11B on trading platform
- bc1 not supported by OMNI, native segwig support needed for omni tokens HOT 1
- it will override all the other settings to false each time of iterator HOT 1
- Too many open files error HOT 2
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from omnicore.