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

Conversation

@0xCLARITY
Copy link
Collaborator

High Level Overview of Change

Adds a version key to the PaymentInformation response payload.

Context of Change

This is needed to cut the PayID-Version and PayID-Server-Version headers from the PayID Protocol when we hit 2.0. I spoke with Aanchal about versioning, and she and I came to the conclusion that (for the moment) we don't even need version negotiation, so we don't need to build out versioning in the PayID Discovery WebFinger JRD like I suggested. You can get all the information you need to know from the version key in the payload itself, and (hopefully) there shouldn't be many major version changes for the foreseeable future after we cut 2.0.

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

The PaymentInformation response payload now includes a version key of the form version: MAJOR.MINOR.

Test Plan

Tests have been updated and all tests pass.

Copy link
Member

@dino-rodriguez dino-rodriguez left a comment

Choose a reason for hiding this comment

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

LGTM!

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 just one refactor Q


const USER = '/alice'
const PAYID = `${USER.slice(1)}$127.0.0.1`
const VERSION = '1.1'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we establish a global const like this one in the rest of the test files in this PR so that we are always using VERSION instead of typing out '1.1' for each?

Copy link
Member

Choose a reason for hiding this comment

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

Any chance we can do this refactor in a subsequent PR so we can get this release cut?

@dino-rodriguez
Copy link
Member

dino-rodriguez commented Aug 18, 2020

Actually one comment - I noticed that you mentioned we don't need version negotiation. That means that when we go from 1.x to 2.0, we are going to break the network, right?

@dino-rodriguez dino-rodriguez mentioned this pull request Aug 18, 2020
7 tasks
@0xASK 0xASK self-requested a review August 18, 2020 21:58
@dino-rodriguez dino-rodriguez merged commit cdee51d into master Aug 18, 2020
@dino-rodriguez dino-rodriguez deleted the add-version-to-payment-information branch August 18, 2020 22:01
0xASK pushed a commit that referenced this pull request Aug 18, 2020
Co-authored-by: Dino Rodriguez <dinoorodriguez@gmail.com>
sasaki-san pushed a commit to jxpug/payid-serverless that referenced this pull request Nov 16, 2020
Co-authored-by: Dino Rodriguez <dinoorodriguez@gmail.com>
sasaki-san added a commit to jxpug/payid-serverless that referenced this pull request Nov 16, 2020
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