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

move bitcoin utils to this repo #28

Merged
merged 2 commits into from
Jun 27, 2024
Merged

move bitcoin utils to this repo #28

merged 2 commits into from
Jun 27, 2024

Conversation

szhygulin
Copy link
Contributor

@szhygulin szhygulin commented Jun 25, 2024

Move bitcoin utils togeather with its tests to this repo. This PR mostly copy-paste BitcoinUtils.sol from stroom-contracts. The only big change is introduction of BitcoinNetworkEncoder library, that is needed to unify network encoding enum across Bech32m and BitcoinUtils contracts.

for (carry = uint8(data_[i]); m > high || carry != 0; m--) {
carry = carry + 256 * uint8(slot[uint256(m)]);
slot[uint256(m)] = bytes1(uint8(carry % 58));
carry /= 58;

Choose a reason for hiding this comment

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

You can list such values as constants at the top for clarity and easier modifications.

function slice(bytes memory data_, uint256 start_, uint256 end_) pure returns (bytes memory) {
unchecked {
bytes memory ret = new bytes(end_ - start_);
for (uint256 i = 0; i < end_ - start_; i++) {

Choose a reason for hiding this comment

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

If Im not mistaking, rewriting this loop using assembly would reduce gas price, but not sure how significant savings might be.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated description of the PR, see above. For answers to these comments, please see below.

*/
function encode(bytes memory data_) pure returns (bytes memory) {
unchecked {
uint256 size = data_.length;

Choose a reason for hiding this comment

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

I believe we can add validation inhere:
require(data_.length > 0, "Input data cannot be empty");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Base58.sol is actually copy-paste of open source lib https://github.com/storyicon/base58-solidity/blob/master/contracts/Base58.sol
we can import it as a submodule, or we can leave it "as is" now and add improvements later

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@diminio1 WDYT?

Comment on lines +7 to +13
bytes constant BTC_BECH32_MAINNET_BYTES = hex"626331"; // prefix = bc1
bytes constant BTC_BECH32_TESTNET_BYTES = hex"746231"; // prefix = tb1
bytes constant BTC_BECH32_REGTEST_BYTES = hex"6263727431"; // prefix = bcrt1

string constant BTC_BECH32_MAINNET = 'bc';
string constant BTC_BECH32_TESTNET = 'tb';
string constant BTC_BECH32_REGTEST = 'brct';
Copy link
Contributor

Choose a reason for hiding this comment

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

is it correct that first 3 constants contain 3 chars and last 3 only 2 chars?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is because it is just copy-paste of Aleksey's code into Mykola's code, keeping their codebases. That's the difference of their code styles :)

string constant BTC_BECH32_TESTNET = 'tb';
string constant BTC_BECH32_REGTEST = 'brct';

//NB: don't forget to update `lnbtc_ext.go` when changing this enum!
Copy link
Contributor

Choose a reason for hiding this comment

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

this file is located in stroom-contracts and renamed already )

@szhygulin szhygulin merged commit 4c27632 into master Jun 27, 2024
1 check passed
@szhygulin szhygulin deleted the ref/move_bitcoin_utils branch June 27, 2024 17:05
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.

3 participants