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

Existing types cleanup + more api methods onto new request method #857

Merged
merged 3 commits into from
Mar 14, 2018

Conversation

FredKSchott
Copy link
Contributor

  • This PR ended up tackling a lot of type clean up and organization
  • getPaymentChannel() now uses this.request()
  • getSettings() now uses this.request()
  • getLedger() now uses this.request()
  • existing transaction types (ledger/types.ts, ledger/transaction-types.ts, etc.) cleaned up, but not yet completely moved to new system

/cc @intelliot

- getPaymentChannel() now uses this.request()
- getSettings() now uses this.request()
- getLedger() now uses this.request()
- transaction types cleaned up a bit, but still some work left to do
- lots of other type cleanup as well
@@ -0,0 +1,56 @@
import {SignerEntry} from './index'

export type PayChannelLedgerEntry = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why use type here instead of interface?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sublimator is right, that's a left over from Flow. Changing to interface...

@sublimator
Copy link
Contributor

sublimator commented Mar 1, 2018 via email

@FredKSchott
Copy link
Contributor Author

@sublimator is right, that's a left over from Flow. Changing to interface...

Copy link
Collaborator

@intelliot intelliot left a comment

Choose a reason for hiding this comment

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

I looked into all the TODOs and while I've learned some things, I don't have resolutions for them yet. We will be adding to the documentation soon, though :)

Meanwhile I think this is safe to merge. I did a couple manual tests and it worked.

@intelliot intelliot merged commit 187154a into XRPLF:develop Mar 14, 2018
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.

3 participants