amiecorso commented 4 years ago

High Level Overview of Change

This PR updates the SDK to handle breaking changes made to PayID. Specifically, the PaymentInformation type was changed to return addresses in the form of an array of Address (new type) objects.

Apologies for the 23-file change - this is intentional because I had to change the name of Address.swift (which was simply a type alias for String) to XRPAddress.swift (and all instances throughout the code) to avoid a name collision with the new PayID type Address. There are also 3 swagger-generated files present (since we have to check in the generated code). Only two files were manually updated WRT breaking PayID changes:

pay-id.v1.yml
PayIDClient.swift

(The XRPPayIDClient unit tests avoid mocked networking in this SDK and therefore, unlike the other two SDKs, didn't need to be updated.)

analogue in JS: https://github.com/xpring-eng/Xpring-JS/pull/474 analogue in Java: https://github.com/xpring-eng/xpring4j/pull/245

@keefertaylor check out my TODO in PayIDClient.

New object structure in payid:

Context of Change

Type of Change

[x] Bug fix (non-breaking change which fixes an issue)

Before / After

PayIDClient unit and integration tests pass again.

Test Plan

CI

Future Tasks

codecov[bot] commented 4 years ago

Codecov Report

Merging #223 into master will increase coverage by 0.02%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #223      +/-   ##
==========================================
+ Coverage   44.77%   44.80%   +0.02%     
==========================================
  Files          62       62              
  Lines        1999     2000       +1     
==========================================
+ Hits          895      896       +1     
  Misses       1104     1104

Impacted Files	Coverage Δ
XpringKit/JavaScript/JavaScriptUtils.swift	`100.00% <100.00%> (ø)`
XpringKit/JavaScript/JavaScriptWallet.swift	`100.00% <100.00%> (ø)`
XpringKit/PayID/PayIDClient.swift	`95.65% <100.00%> (+0.09%)`	:arrow_up:
XpringKit/XRP/DefaultXRPClient.swift	`97.01% <100.00%> (ø)`
XpringKit/XRP/ReliableSubmissionXRPClient.swift	`93.87% <100.00%> (ø)`
XpringKit/XRP/Utils.swift	`96.29% <100.00%> (ø)`
XpringKit/XRP/Wallet.swift	`96.96% <100.00%> (ø)`
XpringKit/XRP/XRPClient.swift	`100.00% <100.00%> (ø)`
...ringKit/ILP/Generated/get_account_request.pb.swift	`0.00% <0.00%> (ø)`
...ringKit/ILP/Generated/get_balance_request.pb.swift	`35.71% <0.00%> (ø)`
... and 6 more

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 2fc2ec1...b075eea. Read the comment docs.

keefertaylor commented 4 years ago

Brutal that we have to rename Address to XRPAddress. Can we note that in the CHANGELOG?

0xCLARITY commented 4 years ago

Brutal that we have to rename Address to XRPAddress. Can we note that in the CHANGELOG?

Would it be easier to make Address => PayIdAddress instead?

amiecorso commented 4 years ago

Brutal that we have to rename Address to XRPAddress. Can we note that in the CHANGELOG?

Would it be easier to make Address => PayIdAddress instead?

I think the issue with that is the fact that Address is an object that's part of the actual PayID protocol... so I feel like renaming Address to PayIdAddress actually has deeper implications than renaming Address to XRPAddress. Though maybe it's ok to use the name PayIdAddress in the local implementation without any consequences, because the structure of the protocol remains? I think I need @keefertaylor 's verdict on this one...