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

Output Bech32Address for initial accounts #594

Merged
merged 5 commits into from
Sep 23, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions fuel-core/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ async-graphql = { version = "4.0", features = [
], default-features = false }
async-trait = "0.1"
axum = { version = "0.4" }
bech32 = "0.9.0"
bincode = "1.3"
byteorder = "1.4.3"
chrono = { version = "0.4", features = ["serde"] }
Expand Down
14 changes: 13 additions & 1 deletion fuel-core/src/chain_config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@ use crate::{
database::Database,
model::BlockHeight,
};
use bech32::{
ToBase32,
Variant::Bech32m,
};
use fuel_core_interfaces::{
common::{
fuel_tx::ConsensusParameters,
Expand Down Expand Up @@ -41,6 +45,9 @@ use std::{
str::FromStr,
};

// Fuel Network human-readable part for bech32 encoding
pub const FUEL_BECH32_HRP: &str = "fuel";
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm. Is there some way to import the SDK instead of duplicating code? If the SDK isn't set up for that, then we can merge as-is then have a follow-up good first issue to clean up.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we can import from the SDK as it is currently declared as public but do we want to depend on the SDK from fuel-core? If that is acceptable, we will need to declare fuels-types as a dependency and after that we can fetch it from there

Copy link
Member

Choose a reason for hiding this comment

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

The bech32 encoding logic for Address should probably reside in fuel-types.

It's also worth noting that none of the fuel-core GraphQL APIs currently support bech32, so hijacking the output format here for the SDK would make them unusable for local client dev just using the graphql API directly.

We should decide whether base16 or bech32 is the standard encoding for addresses, since this is going to cause more confusion for the foreseeable future.

Copy link
Member

Choose a reason for hiding this comment

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

@kayagokalp for now, let's just return both hex and bech32 format for the address until we settle on a standard approach.


pub mod serialization;

pub const LOCAL_TESTNET: &str = "local_testnet";
Expand Down Expand Up @@ -80,10 +87,15 @@ impl ChainConfig {
&mut rng,
);
let address = Address::from(*secret.public_key().hash());
let bech32_data = Bytes32::new(*address).to_base32();
adlerjohn marked this conversation as resolved.
Show resolved Hide resolved
let bech32_encoding =
bech32::encode(FUEL_BECH32_HRP, &bech32_data, Bech32m).unwrap();

tracing::info!(
"PrivateKey({:#x}), Address({:#x}), Balance({})",
"PrivateKey({:#x}), Address({:#x} [bech32: {}]), Balance({})",
secret,
address,
bech32_encoding,
TESTNET_INITIAL_BALANCE
);
CoinConfig {
Expand Down