Skip to content
This repository was archived by the owner on Mar 8, 2024. It is now read-only.

Conversation

@dino-rodriguez
Copy link
Member

High Level Overview of Change

Add verifiedAddress field to PaymentInformation.

Update Public API interface to adhered to updated PaymentInformation type.

Update tests so they continue to pass with the expanded interface.

Context of Change

The goal of this PR is to add verifiedAddresses to the type definition, and to
subsequently update all places in the code so they code compiles & tests
continue to pass.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactor (non-breaking change that only restructures code)
  • Tests (You added tests for code that already exists, or your new feature included in this PR)
  • Documentation Updates
  • Release

Before / After

Before: PaymentInformation does not contain verifiedAddresses
After: PaymentInformation contains verifiedAddresses

Test Plan

This PR updates all tests so they continue to pass.

I will add any necessary new tests in a subsequent PR.

addressDetails: address.details,
}
}),
verifiedAddresses: [],
Copy link
Member Author

Choose a reason for hiding this comment

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

For now, this will just be an empty response.

@@ -1,3 +1,8 @@
/* eslint-disable import/no-cycle --
Cycle between this file and types/verifiedAddress.ts. Should we combine these into one?
Copy link
Member Author

Choose a reason for hiding this comment

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

Currently we have a types file for verified addresses, and one for the public API.

The other two type files are database types.

I'm thinking that the verified addresses types, and public api types, are really all just protocol types.

I'm thinking we should combine these into one file like (types/protocol.ts or types/address.ts). This would require a bit of a refactor to all the references to previous types files, so I would do that as a subsequent PR.

What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ya I'm into that, these have been calling for a refactor for a minute now

},
},
],
verifiedAddresses: [],
Copy link
Member Author

Choose a reason for hiding this comment

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

Updates a bunch of tests so they continue to pass.

Copy link
Collaborator

@0xASK 0xASK left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -1,3 +1,8 @@
/* eslint-disable import/no-cycle --
Cycle between this file and types/verifiedAddress.ts. Should we combine these into one?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ya I'm into that, these have been calling for a refactor for a minute now

@dino-rodriguez dino-rodriguez merged commit 9952f21 into master Aug 3, 2020
@dino-rodriguez dino-rodriguez deleted the dr-vpayid-types branch August 3, 2020 21:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants