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.

Sign up for GitHub

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

Jump to bottom

[2.0] New implementation of addr_canonicalize and addr_humanize functions #1914

Merged
merged 25 commits into from
Dec 13, 2023
Merged

[2.0] New implementation of addr_canonicalize and addr_humanize functions #1914

merged 25 commits into from
Dec 13, 2023

Conversation

DariuszDepta
Copy link
Member

@DariuszDepta DariuszDepta commented Oct 10, 2023 •
edited by chipshort
Loading

closes #1873
closes #1648

@DariuszDepta DariuszDepta linked an issue Oct 10, 2023 that may be closed by this pull request
New implementation for addr_canonicalize and addr_humanize functions #1873
Closed
@DariuszDepta DariuszDepta marked this pull request as draft October 10, 2023 11:15
@DariuszDepta DariuszDepta force-pushed the 1873-new-impl-canonicalize-humanize branch from 4729cf7 to 3d3208e Compare November 27, 2023 11:25
@chipshort chipshort force-pushed the 1873-new-impl-canonicalize-humanize branch from c806cb6 to 6ec5ea0 Compare December 4, 2023 10:28
@chipshort chipshort force-pushed the 1873-new-impl-canonicalize-humanize branch from 5711f3d to 966bd0a Compare December 4, 2023 12:09
@chipshort
Copy link
Collaborator

I ported the new MockApi over to the vm and fixed all the remaining failing tests.
Some notes:

  • I added length limits for the addresses: 1 <= canonical_bytes <= 255 as discussed
  • I had to remove some of the vm tests with addresses that are too long because the humanized address of a 256 byte canonical address is bigger than the max region length for humanized addresses (which is 256 characters)
  • I did not change all tests to use addr_make, only the ones that were failing because of invalid addresses.

@chipshort chipshort marked this pull request as ready for review December 4, 2023 12:32
webmaster128
webmaster128 reviewed Dec 5, 2023
View reviewed changes
Copy link
Member

@webmaster128 webmaster128 left a comment

Choose a reason for hiding this comment

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

Very nice 🙌

contracts/hackatom/tests/integration.rs Outdated Show resolved Hide resolved
packages/std/src/testing/mock.rs Outdated Show resolved Hide resolved
packages/std/src/testing/mock.rs Outdated Show resolved Hide resolved
packages/std/src/testing/mock.rs Outdated Show resolved Hide resolved
packages/std/src/testing/mock.rs Outdated Show resolved Hide resolved
packages/std/src/testing/mock.rs Outdated Show resolved Hide resolved
packages/vm/Cargo.toml Show resolved Hide resolved
packages/vm/src/testing/mock.rs Outdated Show resolved Hide resolved
packages/vm/src/testing/mock.rs Outdated Show resolved Hide resolved
packages/vm/src/testing/mock.rs Outdated Show resolved Hide resolved
@webmaster128
Copy link
Member

webmaster128 commented Dec 5, 2023 •
edited
Loading

Also we should add an entry to MIGRATING.md to explain the most relevant contract test changes required by this PR

@chipshort
Copy link
Collaborator

I added changelog and migration entries. Also added a test verifying that this PR closes #1648

webmaster128
webmaster128 approved these changes Dec 13, 2023
View reviewed changes
Copy link
Member

@webmaster128 webmaster128 left a comment

Choose a reason for hiding this comment

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

🙌

@webmaster128 webmaster128 merged commit 0aab099 into main Dec 13, 2023
29 checks passed
@webmaster128 webmaster128 deleted the 1873-new-impl-canonicalize-humanize branch December 13, 2023 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chipshort chipshort chipshort left review comments

@webmaster128 webmaster128 webmaster128 approved these changes

Assignees

@chipshort chipshort

Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@DariuszDepta @chipshort @webmaster128