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

Allow rest base 32 addresses #804

Merged
merged 1 commit into from
Oct 5, 2021
Merged

Allow rest base 32 addresses #804

merged 1 commit into from
Oct 5, 2021

Conversation

fboucquez
Copy link
Contributor

@fboucquez fboucquez commented Jul 26, 2021

This is the SDK side of symbol/catapult-rest#611

The new rest mappers will accept both encoded/hex and plain/decoded addresses. It will buffer the migration allowing the SDK rest client to be compatible with old and new rests. Eventually, we should only allow base32 addresses. Apps only need to upgrade the SDK, no code change is required.

A migration plan could be:

  • Merge and release this SDK fix.
  • Merge and release the Rest fix.
  • Upgrade SDKs in all our apps (CLIs, explorers, wallets, etc). Release the apps. The apps will be old and new rest compatible.
  • Migrate testnet, mainnet, and community nodes to the new rest when possible.

No flag is required.

Related issues:
#801
#802

@fboucquez fboucquez requested a review from rg911 July 26, 2021 10:56
@fboucquez fboucquez changed the title Rest plain addresses Allow rest plain/decoded addresses Jul 26, 2021
@fboucquez fboucquez changed the base branch from main to dev July 26, 2021 10:59
@fboucquez fboucquez changed the title Allow rest plain/decoded addresses Allow rest base 32 addresses Oct 1, 2021
@sonarcloud
Copy link

sonarcloud bot commented Oct 1, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
1.6% 1.6% Duplication

* @return the Address object.
*/
public static toAddress(value: string): Address {
return Convert.isHexString(value, 48) ? Address.createFromEncoded(value) : Address.createFromRawAddress(value);
Copy link
Member

@AnthonyLaw AnthonyLaw Oct 3, 2021

Choose a reason for hiding this comment

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

Example in transfer transaction, If the recipientAddress is alias THBIMC3THGH5RUYAAAAAAAAAAAAAAAAAAAAAAAA

It will throw an error Address Network unsupported right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We haven't removed private/mijin network support from the old SDKs yet. Unsure if it makes sense to remove them, this SDK would fade off eventually.

Copy link
Member

Choose a reason for hiding this comment

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

Ignore my question.
Actually, I misunderstood, even though the alias namespace is still converted to a valid address format from REST.

@fboucquez fboucquez merged commit 82bd669 into dev Oct 5, 2021
@fboucquez fboucquez deleted the rest_plain_addresses branch October 5, 2021 13:11
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.

2 participants