bzp99
commented
1 year ago The following and similar nested statements should be split into multiple statements using variable assignments, so it's easier to read (and would facilitate step-by-step debugging if we could set that up 😄).
I would consider this a very low-priority issue, but I slightly disagree. This is subjective, but I think serialize(doSomething(deserialize(params))) is still within the realms of readability. But maybe I just like Lisp too much :laughing:.
Anyway, my main point is that from a debugging perspective, I think decent debuggers (which are available for Java) let you step into statements even if they are inlined like this. So this is a non-issue. Which makes all this a matter of preference, so if others prefer the expanded form, I don’t mind. Just wanted to add my 2c.
[other points]
I agree with everything else, thanks for the insights! The mentioned method names are indeed misleading and most importantly, I am well aware of the poor implementation of Registry#select. This is what we could afford in the available time, but it’s a clear point of improvement with surely significant performance gain effects.
Legend: 🔴: High priority. 🟡: Medium priority. 🟢: Low priority. 🔵: Discussion.
General
TPCCclass could be namedTPCCContractAPI(orTpccContractApi, not sure about the commonly used casing), and the strongly typed methods could be extracted into aTpccBusinessApiclass, which now deals purely with business-level objects, not Fabric API-enforced strings. https://github.com/ftsrg/blockchain-benchmarks-tpcc/blob/385c149a293c016a0da75f646f67f8a6b2a488a0/smart-contract/hyperledger-fabric/v2/java/src/main/java/hu/bme/mit/ftsrg/chaincode/tpcc/TPCC.java#L49Delivery
getOldestNewOrderForDistrictmethod is misleading. It doesn't return the oldest new order, but actually "performs" the delivery. It should either be renamed to something likedeliverOldestNewOrderForDistrictto reflect its functionality or its contents should be inlined into the shortdeliverymethod, except for thegetOldestNewOrderfunctionality (just like in the JS version) since that's the "abstraction border" between the business-level and registry-level functionality. https://github.com/ftsrg/blockchain-benchmarks-tpcc/blob/385c149a293c016a0da75f646f67f8a6b2a488a0/smart-contract/hyperledger-fabric/v2/java/src/main/java/hu/bme/mit/ftsrg/chaincode/tpcc/TPCC.java#L349selectimplementation now reads every entry of a given type, then it's filtered inside the chaincode usingmatching: https://github.com/ftsrg/blockchain-benchmarks-tpcc/blob/385c149a293c016a0da75f646f67f8a6b2a488a0/smart-contract/hyperledger-fabric/v2/java/src/main/java/hu/bme/mit/ftsrg/chaincode/tpcc/TPCC.java#L1201 This is really wasteful if it works that way and could cause a lot of MVCC conflicts. The originalselectsupported a partial iterator and early-break functionality for such scenarios (we only need the first/earliest new order of a given warehouse and district). The implementation is functionally correct, but not robust enough extra-functionally.getOrderLineAmountfunction also does more than its name suggests: https://github.com/ftsrg/blockchain-benchmarks-tpcc/blob/385c149a293c016a0da75f646f67f8a6b2a488a0/smart-contract/hyperledger-fabric/v2/java/src/main/java/hu/bme/mit/ftsrg/chaincode/tpcc/TPCC.java#L1264New Order
dist_infoattribute must be assigned the value of the requireds_dist_xxattribute from theStockinstance. https://github.com/ftsrg/blockchain-benchmarks-tpcc/blob/119b79fa5344215ae2d1f3f3c38938addf440c84/smart-contract/hyperledger-fabric/v2/java/src/main/java/hu/bme/mit/ftsrg/chaincode/tpcc/TPCC.java#L1504 See here: https://github.com/ftsrg/blockchain-benchmarks-tpcc/blob/119b79fa5344215ae2d1f3f3c38938addf440c84/smart-contract/hyperledger-fabric/v1/javascript/lib/tpcc.js#L361Order Status
Payment
LGTM
Stock Level
Other components
TBD