Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove deprecated ledger fields #1160

Merged
merged 2 commits into from
Jul 26, 2021
Merged

Conversation

intelliot
Copy link
Collaborator

Per XRPLF/rippled#3214

The following fields are deprecated and may be removed without further notice: accepted, hash (use ledger_hash instead), seqNum (use ledger_index instead), totalCoins (use total_coins instead).

Verified

This commit was signed with the committer’s verified signature.
rexf Rex Fong
In all of the other places we import BigNumber, we make use of the
default export. This just makes it consistent across the project.

export default BigNumber;

Verified

This commit was signed with the committer’s verified signature.
rexf Rex Fong
Per XRPLF/rippled#3214

The following fields are deprecated and may be removed without further notice: accepted, hash (use ledger_hash instead), seqNum (use ledger_index instead), totalCoins (use total_coins instead).
@FKSRipple
Copy link
Collaborator

Can you remove from relevant code / type definitions as well? Ex: I see seqNum used in:

  • src/offline/ledgerhash.ts
  • src/ledger/parse/ledger.ts
  • src/common/types/objects/ledger.ts

@intelliot intelliot self-assigned this Jan 16, 2020
@intelliot intelliot removed their assignment Mar 31, 2021
@intelliot intelliot self-assigned this Jul 7, 2021
@intelliot
Copy link
Collaborator Author

Can you remove from relevant code / type definitions as well? Ex: I see seqNum used in:

  • src/offline/ledgerhash.ts
  • src/ledger/parse/ledger.ts
  • src/common/types/objects/ledger.ts

Done

@intelliot intelliot requested review from mvadari and natenichols July 26, 2021 17:48
@intelliot
Copy link
Collaborator Author

This might technically be a breaking change, but these fields are directly redundant with the snake_case versions and have been deprecated since ~2015. So I think this would be fine to merge

@@ -67,19 +66,19 @@ function parseState(state) {
* @throws RangeError: Invalid time value (rippleTimeToISO8601)
*/
export function parseLedger(ledger: Ledger): FormattedLedger {
const ledgerVersion = parseInt(ledger.ledger_index || ledger.seqNum, 10)
Copy link
Contributor

@natenichols natenichols Jul 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we using parseInt(..., 10), could we just use Number(...) if parsing in base 10?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No reason. I suspect Number() would work equally well, but I'm not 100% sure. It has been parseInt() from the beginning of time and I think we can leave it that way unless there is some reason to change it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm perfectly happy with parseInt(), mostly just curious.

@intelliot intelliot merged commit aa081a4 into develop Jul 26, 2021
@intelliot intelliot deleted the remove-deprecated-ledger-fields branch July 26, 2021 23:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants