Task/604 address formatting #611

AnthonyLaw · 2021-05-06T15:14:00Z

Fix

added encodedAddress in modelType.
added hexToUint8Reverse (use for NamespaceId) in convert class.
added bufferToResolvedAddress function to convert buffer to Base32 Address and NamespaceId

// Address
"recipientAddress":"TDYGHCCOUL5RB6XOBWN466ZQRGS2FFYVTOCBRNI"

// NamespaceID
"recipientAddress": "BA25CF1CD00FABCA",

Todo:

Resolved: #604

mm-s · 2021-05-10T18:45:08Z

what is the rationale for replacing binary with encoded format?
Thanks

AnthonyLaw · 2021-05-11T09:02:49Z

what is the rationale for replacing binary with encoded format?
Thanks

To convert the address field to encoded format is easy for a developer to view data directly into REST return.

Example:

68488A227444FC58A7E9740461071B478DBE52720C3E77A6 -> current REST Return.
NBEIUITUIT6FRJ7JOQCGCBY3I6G34UTSBQ7HPJQ -> encoded format

mm-s · 2021-05-11T12:37:47Z

"directly into REST return"

Since the field name has been reused, how does the change ensure not breaking existing REST service clients? How about external apps that potentially we don't even know they exist?

mm-s · 2021-05-11T12:52:55Z

I always critizise loading backends with formatting features when the only purpose is making it easier or more readable for the user, because it comes at the expense of increasing the workload of a backend service, where little penalties can produce big effects as they serve many users.

rg911 · 2021-05-17T16:48:50Z

This seems to be a 'breaking' change to all the client apps. But worth having It imo. We Will discuss with other teams see how we can handle the impact

gimre-xymcity · 2021-05-17T17:27:37Z

@mm-s the problem is that this "binary" isn't actually binary, it's hex-encoded which makes it longer than usual base32 we use to represent as strings...

but yeah, if it's breaking change, shouldn't there be "old" / "new" APIs with "old" being deprecated (and removed in some future update?)

fboucquez · 2021-05-17T18:00:30Z

The SDKs will handle the impact for clients using the SDKs. All our apps are using SDKs. SDK hides how an unresolved address is mapped to an Address/Alias Object. The mappers will create the objects from friendly addresses and alias Ids instead of the hex values.

The release needs to be synchronized with the SDKs upgrades. Unresolved Addresses are everywhere, so probably we would need both APIs to work at the same time

AnthonyLaw · 2021-06-01T14:54:53Z

I think this PR definitely needs to synchronize with the SDKs upgrades, I don't think it's urgent to merge.

Do we need to support hex-encoded address and base32 address on API in the future?

If Yes, then will add encodeAddress flag in req params for all the endpoint, to allow changing the address format.
Else it worth for wait for the SDK upgrade.

fboucquez · 2021-07-26T13:11:23Z

Related PRs
symbol/symbol-openapi#272
symbol/symbol-sdk-typescript-javascript#804

fboucquez · 2021-07-26T13:44:22Z

I think receipts' resolutions are missed....

https://github.com/AnthonyLaw/catapult-rest/blob/task/604_address-formatting/catapult-sdk/src/plugins/receipts.js#L53
https://github.com/AnthonyLaw/catapult-rest/blob/task/604_address-formatting/catapult-sdk/src/plugins/receipts.js#L70

@AnthonyLaw

fboucquez · 2021-07-26T13:52:56Z

rest/src/db/dbUtils.js

 	 */
-	bufferToAddressBase32: addressBinary => {
-		const hexToUint8 = convert.hexToUint8(addressBinary.toString('hex'));
+	bufferToResolvedAddress: binary => {


the name would be bufferToUnresolvedAddress. The function is returning an Encoded Address or a Namespace Id Hex.

Jaguar0625 · 2021-08-04T19:18:43Z

rest/src/db/dbUtils.js

+	bufferToUnresolvedAddress: binary => {
+		if (!(binary instanceof MongoDb.Binary))
+			return undefined;
+
+		const hex = binary.toString('hex');
+		const bit0 = convert.hexToUint8(hex.substr(1, 2))[0];
+
+		if (16 === (bit0 & 16)) {
+			// only 8 bytes are relevant to resolve the NamespaceId
+			const namespaceId = hex.substr(2, 16);
+
+			// retun as namespace Id
+			return convert.uint8ToHex(convert.hexToUint8Reverse(namespaceId));
+		}
+
+		// return as Address base 32
+		const hexToUint8 = convert.hexToUint8(hex);
+		return address.addressToString(hexToUint8);


why this instead of always addressToString? that is how we normally represent aliases, i think?

for my understanding, addressToString method is the only way to convert Uint8 to an encoded address.

i meant returning alias as namespace id seems strange to me. i don't think we use that representation anywhere else, so i'd just use addressToString

I got what you mean now, the reason to add namespace id is to handle Alias address.

This is the simple.

http://ngl-dual-501.testnet.symboldev.network:3000/transactions/confirmed/3D81C7E22AE0504ABD86EBBBF998E8A1F1F9E6D4C329523ED31760D93B8735C0

What is the expected result, if that is namespace Id hex on recipientAddress?

or we should not allow namespace id to appear on "recipientAddress"?

would it be better to return the encoded address from padded namespaceId, but also have an extra field addressAlias for the actual namespace?
e.g.

{ "recipientAddress": "NATNE7Q5BITMUTRRN6IB4I7FLSDRDWZA35C4KNQ"} or "recipientAddress": "NATNE7Q5BITMUTRRN6IBAAAAAAAAAAAAAAAAAA", "addressAlias": "291F837E059AE13C"}

Atm, Rest, Catbuffer and Mongo represent unresolved addresses like this (hex or byte array):

Address: recipientAddress: 981DFFB44F83E5FA8DE3A564A0A95449848272050AD4F5EC Alias: recipientAddress: 99C1ED94FF2D65C0AF000000000000000000000000000000

When a rest client is processing that payload, it "IFs" to get the right address or namespace id object.

In my mind, this PR just updates that rest payload representation to a "friendly" one:

Address: recipientAddress: TAO77NCPQPS7VDPDUVSKBKKUJGCIE4QFBLKPL3A Alias: recipientAddress: AFC0652DFF94EDC1

A rest client would need to do a similar "IF", just from a different format to get the address or namespace id object. The OLD sdk buffers this changge.

If you check the catbuffer schemas, there are at least 12 attributes that use unresolved addresses (metadata targetAddress, multisig modification addressAdditions, addressDeletions, etc)

Resolving the aliases would be nice, but it is more than just changing the format of the unresolved address value and it needs to be replicated to all those fields. Resolving an alias needs at least a call to the database and the time of the transaction in order to get the right original address.

i like rg911 suggestion to return object with encoded address and optional alias namespace id. or even just always returning encoded address might be sufficient.

i think it would be confusing for consumers to return either encoded address or namespace id.

I believe our goal is to solve the confusion problem for the API.
I will support always returning encoded addresses for the address field will be great.

We need another PR for this.

Always resolve the alias to address (I believe it may needs to sacrifice performance)

rest/src/db/dbFormattingRules.js

rest/src/db/dbUtils.js

rest/src/server/messageFormattingRules.js

rest/test/db/dbUtils_spec.js

catapult-sdk/src/utils/convert.js

rest/test/db/dbUtils_spec.js

fboucquez · 2021-08-10T17:33:15Z

Approved, we would need to review and merge these ones too

symbol/symbol-openapi#272
symbol/symbol-sdk-typescript-javascript#804

Jaguar0625

would prefer to change the alias formatting before merging

fboucquez · 2021-08-25T12:12:55Z

@AnthonyLaw do you have examples of what is this PR rendering for unresolved address fields? What rest returns when recipientAddress is a real address? What rest returns when the recipientAddress is an alias?
I got a bit lost with the changes.

AnthonyLaw · 2021-08-29T11:00:04Z

@fboucquez sorry for the late reply.

recipientAddress is a real address?
Yes.

98E0D138EAF2AC342C015FF0B631EC3622E8AFFA04BFCC56 -> TDQNCOHK6KWDILABL7YLMMPMGYRORL72AS74YVQ

recipientAddress is an alias?

No, but this is what we need to solve: #652
To reduce the confusion, we should only return the Address, rather than the alias.

We should fix issue 652, before merge this.

fboucquez · 2021-08-31T13:02:04Z

rest/src/db/dbUtils.js

+	 * @returns {string} AddressBase32|NamespaceId
+	 */
+	bufferToUnresolvedAddress: binary => {
+		if (!(binary instanceof MongoDb.Binary))


What happens if the binary is an alias? Like 0x99C1ED94FF2D65C0AF000000000000000000000000000000 ?
It seems like the address.addressToString(binary.buffer); only renders address binaries?

Yes, this is only for Addresses.

The output for 0x99C1ED94FF2D65C0AF000000000000000000000000000000 --> THBIMC3THGH5RUYAAAAAAAAAAAAAAAAAAAAAAAA

which is we should not allow alias on the Address field.

Catbuffer allows aliases in the (unresolved)address fields and mongo stores it. Rest needs to render that, AAAA doesn't seem right to me...

fboucquez

I have tested this PR and it doesn't render aliases correctly.

All the "addresses" we are formating here are actually unresolved addresses, where the value can either be a real address or a namespace id.

If you try this PR against testnet, you will see this:

http://localhost:3000/transactions/confirmed?height=141765

The same happens with all the unresolved addresses

The namespace id format is incorrect, it's not either the current hex format or a namespace id

When the "address" field is actually an alias, we should either render the binary hex or the namespace friendly id as explained before, at least in this pr. Resolving the alias could be done in another PR, but this pr should not break the alias rendering. Be aware that resolving the alias would require db access, it's a feature change.

Note: to try testnet:

got to ./rest and run
yarn bootstrap-start-testnet
and then in another terminal
yarn start:dev

Wait for testnet to be synced a little bit, another (lower) example of this issue is:

http://localhost:3000/transactions/confirmed?height=846

You can pull more examples of blocks with aliases

http://localhost:3000/statements/resolutions/address?order=asc

fboucquez · 2021-08-31T13:03:10Z

rest/test/db/dbFormattingRules_spec.js

@@ -239,4 +239,15 @@ describe('db formatting rules', () => {
 			});
 		});
 	});
+
+	it('can format encodedAddress type', () => {


Add a case where the unresolved address is an alias, 0x99C1ED94FF2D65C0AF000000000000000000000000000000

fboucquez · 2021-09-01T16:35:18Z

@Jaguar0625 , do you think that rendering aliases as an invalid address is ok? Please see the screenshots above with all the AAAAs

Jaguar0625 · 2021-09-01T17:18:00Z

yes, that is a valid unresolved address. i'm not sure i see the problem?

gimre-xymcity · 2021-09-01T17:18:06Z

@Jaguar0625 , do you think that rendering aliases as an invalid address is ok? Please see the screenshots above with all the AAAAs

I'm not sure what you're saying, this technically is a valid address... (not valid in the sense of IsValidAddress)
(that is exactly how it looks in transaction itself)

fboucquez · 2021-09-01T17:38:36Z

I'm not sure what you're saying, this technically is a valid address... (not valid in the sense of IsValidAddress)
(that is exactly how it looks in transaction itself)

Yes, I'm talking in the sense of IsValidAddress

This is the spec for the related open API symbol/symbol-openapi#272

https://github.com/symbol/symbol-openapi/blob/4939747f84089bbc640e772e63787ed2405f11f2/spec/schemas/UnresolvedAddress.yml

You can see that the OR links to a NamespaceId.yml which is a user-friendly namespace identifier 16 hex format

https://github.com/symbol/symbol-openapi/blob/4939747f84089bbc640e772e63787ed2405f11f2/spec/schemas/NamespaceId.yml

like: 85BBEA6CC462B244, a 16 chars hex.

I think the AAAA style address could be confusing and it cannot be used as a query or path param like the 16 hex. Examples:

http://ngl-dual-101.testnet.symboldev.network:3000/namespaces (16 hex id)
http://ngl-dual-101.testnet.symboldev.network:3000/namespaces/EE0EDBB5737CA68C (16 hex param)

https://github.com/symbol/symbol-openapi/blob/main/spec/parameters/path/namespaceId.yml
https://github.com/symbol/symbol-openapi/blob/main/spec/schemas/NamespaceId.yml
https://github.com/symbol/symbol-openapi/blob/main/spec/plugins/namespace/routes/getNamespaceMerkle.yml#L7
https://github.com/symbol/symbol-openapi/blob/main/spec/plugins/namespace/routes/getNamespace.yml#L7
https://github.com/symbol/symbol-openapi/blob/main/spec/parameters/query/targetId.yml

One benefit of this PR is that you can use the real address from the payload as a query param, ideally, the namespace id can be used as a query param too (at least until we figure out the alias resolution).

The AAAAA parsing/mapping to a usable namespace id could get a bit funny

If you think the AAAA addresses are fine, ok to merge.

Jaguar0625 · 2021-09-01T23:12:34Z

i think this PR is fine to merge. longer term we might want to consider @rg911 suggestion: #611 (comment)

sonarcloud · 2021-10-01T08:42:08Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
3.3% Duplication

AnthonyLaw marked this pull request as ready for review May 10, 2021 18:05

fboucquez mentioned this pull request Jul 26, 2021

Allow rest base 32 addresses symbol/symbol-sdk-typescript-javascript#804

Merged

fboucquez reviewed Jul 26, 2021

View reviewed changes

AnthonyLaw requested a review from fboucquez August 4, 2021 16:35

AnthonyLaw force-pushed the task/604_address-formatting branch from 98b4fbb to a418822 Compare August 4, 2021 16:40

Jaguar0625 suggested changes Aug 4, 2021

View reviewed changes

Jaguar0625 reviewed Aug 9, 2021

View reviewed changes

AnthonyLaw force-pushed the task/604_address-formatting branch from 8adbd14 to 83fd5e7 Compare August 10, 2021 12:52

AnthonyLaw requested a review from Jaguar0625 August 10, 2021 12:53

Jaguar0625 previously approved these changes Aug 10, 2021

View reviewed changes

Jaguar0625 requested a review from gimre-xymcity August 10, 2021 14:17

gimre-xymcity previously approved these changes Aug 10, 2021

View reviewed changes

fboucquez previously approved these changes Aug 10, 2021

View reviewed changes

AnthonyLaw dismissed stale reviews from fboucquez, gimre-xymcity, and Jaguar0625 via f1cbd39 August 10, 2021 17:58

AnthonyLaw force-pushed the task/604_address-formatting branch from d881b70 to f1cbd39 Compare August 10, 2021 17:58

Jaguar0625 suggested changes Aug 20, 2021

View reviewed changes

add encodedAddress in Model Type

74b5c6d

AnthonyLaw added 2 commits August 23, 2021 00:14

fix: remove unused method hexToUint8Reverse

135343c

style: lint fix

4700469

AnthonyLaw force-pushed the task/604_address-formatting branch from f1cbd39 to 4700469 Compare August 22, 2021 16:15

fix: removed namespace id convert related in bufferToUnresolvedAddress

d83758a

AnthonyLaw mentioned this pull request Aug 22, 2021

Only return address rather than alias. #652

Open

Jaguar0625 previously approved these changes Aug 23, 2021

View reviewed changes

style: lint fix

91eab57

AnthonyLaw dismissed Jaguar0625’s stale review via 91eab57 August 25, 2021 07:23

AnthonyLaw requested a review from Jaguar0625 August 25, 2021 07:28

Jaguar0625 previously approved these changes Aug 25, 2021

View reviewed changes

fboucquez reviewed Aug 31, 2021

View reviewed changes

Merge branch 'dev' into task/604_address-formatting

21e8905

fboucquez suggested changes Sep 1, 2021

View reviewed changes

fix merge

7166b3a

fboucquez dismissed Jaguar0625’s stale review via 7166b3a September 1, 2021 12:25

Jaguar0625 approved these changes Sep 1, 2021

View reviewed changes

AnthonyLaw mentioned this pull request Sep 7, 2021

Transaction sent to expired alias is not loading symbol/explorer#928

Closed

Merge branch 'dev' into task/604_address-formatting

64af498

fboucquez self-requested a review October 5, 2021 13:11

fboucquez approved these changes Oct 5, 2021

View reviewed changes

fboucquez merged commit cea4282 into symbol:dev Oct 5, 2021

AnthonyLaw deleted the task/604_address-formatting branch October 5, 2021 19:01

Task/604 address formatting #611

Task/604 address formatting #611

Conversation

AnthonyLaw commented May 6, 2021 • edited Loading

Fix

mm-s commented May 10, 2021

AnthonyLaw commented May 11, 2021

mm-s commented May 11, 2021

mm-s commented May 11, 2021

rg911 commented May 17, 2021

gimre-xymcity commented May 17, 2021

fboucquez commented May 17, 2021 • edited Loading

AnthonyLaw commented Jun 1, 2021 • edited Loading

fboucquez commented Jul 26, 2021

fboucquez commented Jul 26, 2021 • edited Loading

fboucquez Jul 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnthonyLaw Aug 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnthonyLaw Aug 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnthonyLaw Aug 19, 2021 • edited Loading

Choose a reason for hiding this comment

fboucquez commented Aug 10, 2021 • edited Loading

Jaguar0625 left a comment

Choose a reason for hiding this comment

fboucquez commented Aug 25, 2021 • edited Loading

AnthonyLaw commented Aug 29, 2021 • edited Loading

fboucquez Aug 31, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fboucquez left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fboucquez commented Sep 1, 2021 • edited Loading

Jaguar0625 commented Sep 1, 2021

gimre-xymcity commented Sep 1, 2021 • edited Loading

fboucquez commented Sep 1, 2021 • edited Loading

Jaguar0625 commented Sep 1, 2021

sonarcloud bot commented Oct 1, 2021

AnthonyLaw commented May 6, 2021 •

edited

Loading

fboucquez commented May 17, 2021 •

edited

Loading

AnthonyLaw commented Jun 1, 2021 •

edited

Loading

fboucquez commented Jul 26, 2021 •

edited

Loading

fboucquez Jul 26, 2021 •

edited

Loading

AnthonyLaw Aug 6, 2021 •

edited

Loading

AnthonyLaw Aug 11, 2021 •

edited

Loading

AnthonyLaw Aug 19, 2021 •

edited

Loading

fboucquez commented Aug 10, 2021 •

edited

Loading

fboucquez commented Aug 25, 2021 •

edited

Loading

AnthonyLaw commented Aug 29, 2021 •

edited

Loading

fboucquez Aug 31, 2021 •

edited

Loading

fboucquez left a comment •

edited

Loading

fboucquez commented Sep 1, 2021 •

edited

Loading

gimre-xymcity commented Sep 1, 2021 •

edited

Loading

fboucquez commented Sep 1, 2021 •

edited

Loading