XRPLF / xrpl4j

A 100% Java implementation to interact with the XRP Ledger.
ISC License
86 stars 48 forks source link

LedgerIndex uses UnsignedLong but Transaction uses UnsignedInteger #84

Closed sappenin closed 2 years ago

sappenin commented 3 years ago

The LedgerIndex class emits an UnsignedLong via unsignedLongValue(), but the Transaction class uses an UnsignedInteger to represent lastLedgerSequence.

We should decide which Unsigned number type is best for representing numeric XRP Ledger index values, and be consistent across the library.

nkramer44 commented 3 years ago

@sappenin What if we used LedgerIndex more liberally? There are a bunch of fields that represent a ledger sequence, which makes me think they should all be typed as LedgerIndex.

The only issue I could see with doing this is that someone might inadvertently construct an object using one of the LedgerIndex shortcuts (current, validated, closed) where it doesn't apply, such as in Transaction.lastLedgerSequence. I still think we could type those fields as LedgerIndex and just validate that they aren't using one of the shortcuts.

Thoughts?

sappenin commented 3 years ago

See discussion here: https://github.com/XRPLF/xrpl4j/discussions/100

nkramer44 commented 3 years ago

Example of deprecating fields in favor of new ones which use LedgerIndex:

/**
   * Use this with {@link TransactionRequestParams#maxLedger()} to specify a range of up to 1000 ledger indexes,
   * starting with this ledger (inclusive).
   *
   * <p>If the server cannot find the transaction, it confirms whether it was able to search all the
   * ledgers in this range.
   *
   * @return An optionally-present {@link UnsignedLong} indicating the minimum ledger to search.
   */
  @JsonIgnore
  @Deprecated
  Optional<UnsignedLong> minLedger();

  /**
   * Use this with {@link TransactionRequestParams#minLedger()} to specify a range of up to 1000 ledger indexes,
   * ending with this ledger (inclusive).
   *
   * <p>If the server cannot find the transaction, it confirms whether it was able to search all the ledgers in the
   * requested range.
   *
   * @return An optionally-present {@link UnsignedLong} indicating the maximum ledger to search.
   */
  @JsonIgnore
  @Deprecated
  Optional<UnsignedLong> maxLedger();

  @JsonProperty("min_ledger")
  @Value.Default
  default Optional<LedgerIndex> minimumLedger() {
    return minLedger()
      .map(UnsignedLong::intValue)
      .map(UnsignedInteger::valueOf)
      .map(LedgerIndex::of);
  }

  @JsonProperty("max_ledger")
  @Value.Default
  default Optional<LedgerIndex> maximumLedger() {
    return maxLedger()
      .map(UnsignedLong::intValue)
      .map(UnsignedInteger::valueOf)
      .map(LedgerIndex::of);
  }
nkramer44 commented 2 years ago

This is complete, as Transaction.ledgerIndex is Deprecated, and LedgerIndex will now wrap an UnsignedInteger.