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

[master] Add bech32 utilities and initial migration to bech32 address #2459

Merged
merged 1 commit into from
Mar 9, 2021

Conversation

sunny-teo9000
Copy link

Description

To add Bech32 utilities and allow the following API to accept both base16 and bech32 for now.

GetBalance
GetSmartContractCode
GetSmartContractInit
GetSmartContracts
GetSmartContractState
GetSmartContractSubState

Backward Compatibility

  • This is not a breaking change
  • This is a breaking change

Review Suggestion

Status

Implementation

  • ready for review

Integration Test (Core Team)

Was tested with a local isolated server, testing with the bech32 and base16 address

  • local machine test
  • small-scale cloud test

@github-actions github-actions bot changed the title Add bech32 utilities and initial migration to bech32 address [master] Add bech32 utilities and initial migration to bech32 address Feb 24, 2021
@github-actions github-actions bot added this to PRs in development in Core Feb 24, 2021
bool Bech32::HasZilHrp(const std::string& input) {
if (input.length() < 4) return false;

auto hrp = boost::string_view(input);
Copy link
Contributor

Choose a reason for hiding this comment

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

const auto& hrp

return "";
}

for (size_t i = 0; i < inputSize; ++i) {
Copy link
Contributor

Choose a reason for hiding this comment

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

modernize loop

}

for (size_t i = 0; i < inputSize; ++i) {
char c = input[i];
Copy link
Contributor

Choose a reason for hiding this comment

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

can use const auto& here too


#include "Bech32.h"
#include "libServer/AddressChecksum.h"

Copy link
Contributor

Choose a reason for hiding this comment

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

can use using namespace std in .cpp file

Core automation moved this from PRs in development to PRs needing review Feb 25, 2021
src/libUtils/Bech32.cpp Outdated Show resolved Hide resolved
tests/Utils/Test_Bech32.cpp Outdated Show resolved Hide resolved
tests/Utils/Test_Bech32.cpp Outdated Show resolved Hide resolved
@bb111189
Copy link
Contributor

https://github.com/Zilliqa/cryptoutils/tree/main/lib

Can we use bech32 from cryptoutils? This is to avoid 2 copy of bech32. @vaivaswatha has recently created it and will be using for his VM. I think on the core side, ethash is already using cryptoutils

@vaivaswatha
Copy link
Contributor

https://github.com/Zilliqa/cryptoutils/tree/main/lib

Can we use bech32 from cryptoutils? This is to avoid 2 copy of bech32. @vaivaswatha has recently created it and will be using for his VM. I think on the core side, ethash is already using cryptoutils

Example usage: https://github.com/Zilliqa/scilla-vm/blob/4c58dae89aa3bc1567f21ad1ad9aa952ee089142/libsrtl/ScillaBuiltins.cpp#L608. This function and the next one do decoding and encoding respectively.

src/libServer/LookupServer.cpp Outdated Show resolved Hide resolved
src/libServer/LookupServer.cpp Outdated Show resolved Hide resolved
const unsigned int PAGE_SIZE = 10;
const unsigned int NUM_PAGES_CACHE = 2;
const unsigned int TXN_PAGE_SIZE = 100;
const unsigned int UINT8_ADDR_SIZE = ACC_ADDR_SIZE * 2;
Copy link
Contributor

Choose a reason for hiding this comment

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

should be HEX_ADDR_SIZE

@KaustubhShamshery
Copy link
Contributor

Can we have the unit test again?

@sunny-teo9000 sunny-teo9000 force-pushed the feature/migrationtobech32 branch 2 times, most recently from 3bb0588 to b5d52c7 Compare February 26, 2021 01:54
GetBalance
GetSmartContractCode
GetSmartContractInit
GetSmartContracts
GetSmartContractState
GetSmartContractSubState
Core automation moved this from PRs needing review to PRs approved - ready to merge! Mar 9, 2021
@ansnunez ansnunez merged commit cfdb55a into master Mar 9, 2021
Core automation moved this from PRs approved - ready to merge! to PRs done (merged) Mar 9, 2021
@ansnunez ansnunez deleted the feature/migrationtobech32 branch March 9, 2021 06:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Core
  
PRs done (merged)
Development

Successfully merging this pull request may close these issues.

None yet

5 participants