Java chaincode review for enhanced code-base

[aklenik]

Legend:
🔴: High priority.
🟡: Medium priority.
🟢: Low priority.
🔵: Discussion.

General

• 🟢 Since the TPC-C methods are overloaded based on their parameter types (string or strongly typed/parsed object), a more explicit separation of such methods would make sense. Accordingly, the current TPCC class could be named TPCCContractAPI (or TpccContractApi, not sure about the commonly used casing), and the strongly typed methods could be extracted into a TpccBusinessApi class, which now deals purely with business-level objects, not Fabric API-enforced strings.
  

  blockchain-benchmarks-tpcc/smart-contract/hyperledger-fabric/v2/java/src/main/java/hu/bme/mit/ftsrg/chaincode/tpcc/TPCC.java

  Line 49 in 385c149

  public final class TPCC implements ContractInterface {

• 🟢 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 😄).
  

  blockchain-benchmarks-tpcc/smart-contract/hyperledger-fabric/v2/java/src/main/java/hu/bme/mit/ftsrg/chaincode/tpcc/TPCC.java

  Line 76 in 385c149

  JSON.serialize(this.delivery(ctx, JSON.deserialize(parameters, DeliveryInput.class)));

Delivery

• 🟡 The name of the getOldestNewOrderForDistrict method is misleading. It doesn't return the oldest new order, but actually "performs" the delivery. It should either be renamed to something like deliverOldestNewOrderForDistrict to reflect its functionality or its contents should be inlined into the short delivery method, except for the getOldestNewOrder functionality (just like in the JS version) since that's the "abstraction border" between the business-level and registry-level functionality.
  

  blockchain-benchmarks-tpcc/smart-contract/hyperledger-fabric/v2/java/src/main/java/hu/bme/mit/ftsrg/chaincode/tpcc/TPCC.java

  Line 349 in 385c149

  this.getOldestNewOrderForDistrict(

• 🔴 🔵 If I understand correctly, the select implementation now reads every entry of a given type, then it's filtered inside the chaincode using matching:
  

  blockchain-benchmarks-tpcc/smart-contract/hyperledger-fabric/v2/java/src/main/java/hu/bme/mit/ftsrg/chaincode/tpcc/TPCC.java

  Line 1201 in 385c149

  .select(ctx, new NewOrder())

  
  This is really wasteful if it works that way and could cause a lot of MVCC conflicts. The original select supported 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.
• 🟡 The getOrderLineAmount function also does more than its name suggests:
  

  blockchain-benchmarks-tpcc/smart-contract/hyperledger-fabric/v2/java/src/main/java/hu/bme/mit/ftsrg/chaincode/tpcc/TPCC.java

  Line 1264 in 385c149

  this.getOrderLineAmount(ctx, w_id, d_id, order.getO_id(), i, ol_delivery_d);

New Order

• 🟡 I find single-statement for/if/etc blocks without curly brackets really error-prone. We should always use them for clarity, and in the name of defensive programming. The following is just an example
  

  blockchain-benchmarks-tpcc/smart-contract/hyperledger-fabric/v2/java/src/main/java/hu/bme/mit/ftsrg/chaincode/tpcc/TPCC.java

  Line 490 in 119b79f

  for (int i = 0; i < input.getI_ids().length; ++i)

  
  

  blockchain-benchmarks-tpcc/smart-contract/hyperledger-fabric/v2/java/src/main/java/hu/bme/mit/ftsrg/chaincode/tpcc/TPCC.java

  Lines 1446 to 1447 in 119b79f

  	if (stock.getS_quantity() >= i_qty + 10) stock.decreaseQuantity(i_qty);
  	else stock.setS_quantity(stock.getS_quantity() - i_qty + 91);

• 🔴 The existence of a referenced item must be checked (i.e., the exception handled) because it determines the return value of the transaction.
  

  blockchain-benchmarks-tpcc/smart-contract/hyperledger-fabric/v2/java/src/main/java/hu/bme/mit/ftsrg/chaincode/tpcc/TPCC.java

  Line 1427 in 119b79f

  registry.read(ctx, item);

  
  See here:
  

  blockchain-benchmarks-tpcc/smart-contract/hyperledger-fabric/v1/javascript/lib/tpcc.js

  Line 287 in 119b79f

  if (!item) {

• 🔴 The dist_info attribute must be assigned the value of the required s_dist_xx attribute from the Stock instance.
  

  blockchain-benchmarks-tpcc/smart-contract/hyperledger-fabric/v2/java/src/main/java/hu/bme/mit/ftsrg/chaincode/tpcc/TPCC.java

  Line 1504 in 119b79f

  .dist_info(padDistrictInfo("s_dist_" + stockDistrictId))

  
  See here:
  

  blockchain-benchmarks-tpcc/smart-contract/hyperledger-fabric/v1/javascript/lib/tpcc.js

  Line 361 in 119b79f

  ol_dist_info: stock[`s_dist_${stockDistrictId}`]

Order Status

• 🟡 Single statement blocks without curly braces:
  

  blockchain-benchmarks-tpcc/smart-contract/hyperledger-fabric/v2/java/src/main/java/hu/bme/mit/ftsrg/chaincode/tpcc/TPCC.java

  Lines 1568 to 1569 in 119b79f

  	if (c_id == null && c_last == null)
  	throw new IllegalArgumentException("At least one of c_id and c_last must be specified");

  
  

  blockchain-benchmarks-tpcc/smart-contract/hyperledger-fabric/v2/java/src/main/java/hu/bme/mit/ftsrg/chaincode/tpcc/TPCC.java

  Lines 1583 to 1584 in 119b79f

  	for (final Customer c : allCustomers)
  	if (c.getC_last().equals(c_last)) matchingCustomers.add(c);

  
  

  blockchain-benchmarks-tpcc/smart-contract/hyperledger-fabric/v2/java/src/main/java/hu/bme/mit/ftsrg/chaincode/tpcc/TPCC.java

  Lines 1585 to 1586 in 119b79f

  	if (matchingCustomers.isEmpty())
  	throw new NotFoundException("Customer matching last name '%s' not found".formatted(c_last));

  
  

  blockchain-benchmarks-tpcc/smart-contract/hyperledger-fabric/v2/java/src/main/java/hu/bme/mit/ftsrg/chaincode/tpcc/TPCC.java

  Line 1589 in 119b79f

  if (N > Integer.MAX_VALUE) logger.warn("Size of matching CUSTOMER list is out of range");

  
  

  blockchain-benchmarks-tpcc/smart-contract/hyperledger-fabric/v2/java/src/main/java/hu/bme/mit/ftsrg/chaincode/tpcc/TPCC.java

  Line 1617 in 119b79f

  if (allOrders.isEmpty()) throw new NotFoundException("No orders found");

  
  

  blockchain-benchmarks-tpcc/smart-contract/hyperledger-fabric/v2/java/src/main/java/hu/bme/mit/ftsrg/chaincode/tpcc/TPCC.java

  Line 1621 in 119b79f

  for (final Order o : allOrders) if (o.getO_c_id() == o_c_id) matchingOrders.add(o);

  
  

  blockchain-benchmarks-tpcc/smart-contract/hyperledger-fabric/v2/java/src/main/java/hu/bme/mit/ftsrg/chaincode/tpcc/TPCC.java

  Lines 1622 to 1623 in 119b79f

  	if (matchingOrders.isEmpty())
  	throw new NotFoundException("Could not find last order of customer");

• 🔴 Sorting by customer first name is missing:
  

  blockchain-benchmarks-tpcc/smart-contract/hyperledger-fabric/v2/java/src/main/java/hu/bme/mit/ftsrg/chaincode/tpcc/TPCC.java

  Line 1588 in 119b79f

  final double N = Math.ceil(matchingCustomers.size() / 2d);

  
  See here:
  

  blockchain-benchmarks-tpcc/smart-contract/hyperledger-fabric/v1/javascript/lib/ledgerUtils.js

  Line 347 in 119b79f

  customerList.sort((c1, c2) => c1.c_first.localeCompare(c2.c_first));

Payment

LGTM

Stock Level

• 🟢 The minimum constraint should be propagated back to the JS implementation to make it more robust:
  

  blockchain-benchmarks-tpcc/smart-contract/hyperledger-fabric/v1/javascript/lib/tpcc.js

  Line 637 in 119b79f

  const o_id_min = district.d_next_o_id - 20;

• 🟡 Single statement block without curly braces:
  

  blockchain-benchmarks-tpcc/smart-contract/hyperledger-fabric/v2/java/src/main/java/hu/bme/mit/ftsrg/chaincode/tpcc/TPCC.java

  Line 1677 in 119b79f

  if (itemIds.isEmpty()) throw new NotFoundException("Could not find item IDs of recent ORDERs");

Other components

TBD

[bzp99]

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

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.

[bzp99]

A sidenote regarding misleading method names: the javadoc comments above some of the methods indicate that there are side effects, but there are some methods that are missing these comments. Furthermore, just because the comments warn of the side effects, this is still just bad practice.

[bzp99]

Fixed some of the issues in #23 , marking them here…

[bzp99]

We still have the issue of the select implementation being inefficient. IIRC it has been implemented this way because of some OpenJML problems – we just went with this ‘brute-force’ approach for now. I will come back to it later, for now I did not attempt to play with fire again :)

Regarding propagating the minimum constraint back to the JS implementation: I did not do this yet because first I would like to find out why we are checking the last 5 items in the Java version and not the last 20. The spec says 20. I think this also may have been a shortcut to deal with some OpenJML problems or just some simplification for testing, but I am not sure.