xpring-eng / XpringKit

XpringKit provides a Swift SDK for interacting with Xpring Protocols (XRP/PayID/ILP). This library is deprecated.
https://xrpl.org/
MIT License
26 stars 14 forks source link

Fix semantics of methods for current open ledger and last validated ledger #216

Closed keefertaylor closed 4 years ago

keefertaylor commented 4 years ago

High Level Overview of Change

The current implementation of getLatestValidatedLedgerSequence returns the latest open ledger sequence.

Fix this method's name and re-implement getLatestValidatedLedgerSequence.

Context of Change

There are a number of assumptions required to get getLatestValidatedLedgerSequence working. Namely, an address that is guaranteed to exist needs to be provided.

I've worked with @carlhua and @cjcobb23 on the rippled team and we are convinced that this implementation will never fail to return the latest validated ledger sequence when called from our reliable submission logic.

Type of Change

Before / After

Reliable submission bug is fixed.

Test Plan

CI

codecov[bot] commented 4 years ago

Codecov Report

Merging #216 into master will increase coverage by 0.57%. The diff coverage is 91.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #216      +/-   ##
==========================================
+ Coverage   44.14%   44.71%   +0.57%     
==========================================
  Files          62       62              
  Lines        1971     1997      +26     
==========================================
+ Hits          870      893      +23     
- Misses       1101     1104       +3     
Impacted Files Coverage Δ
XpringKit/XRP/ReliableSubmissionXRPClient.swift 93.87% <85.00%> (-6.13%) :arrow_down:
XpringKit/XRP/DefaultXRPClient.swift 97.01% <100.00%> (+0.26%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 661187e...0821b39. Read the comment docs.

keefertaylor commented 4 years ago

Very nice. Question: is there any reason for parameterizing getLatestValidatedLedgerSequence with an address, when you could hard-code the genesis address instead? it seems less likely than any other address to have been deleted or somehow not exist (though I suppose it puts some responsibility on us v.s. the user in terms of picking an extant address..)

The genesis address can be deleted, because someone rotated the keys on the account and they have control of the account.

On the other hand, we're sure that the address which sent a transaction will exist for the duration of the time that the transaction is in flight (unless the account is deleted in a race condition, in which case the transaction will fail anyway).

Then there's just the piece we discussed offline about potentially returning (hash, status) tuples from this method instead of just a hash that doesn't tell you anything about the actual ultimate status of the transaction, but that would be a separate PR anyway

Yup, trying to keep this to be a minimal change.