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

Conversation

kayagokalp
Copy link
Member

closes #593.

@kayagokalp kayagokalp added the enhancement New feature or request label Sep 5, 2022
@kayagokalp kayagokalp requested review from adlerjohn and a team September 5, 2022 03:39
@kayagokalp kayagokalp self-assigned this Sep 5, 2022
@@ -14,6 +15,9 @@ use serde_with::{serde_as, skip_serializing_none};
use serialization::{HexNumber, HexType};
use std::{io::ErrorKind, path::PathBuf, 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.

@kayagokalp kayagokalp enabled auto-merge (squash) September 16, 2022 09:07
@kayagokalp kayagokalp merged commit 96e526e into master Sep 23, 2022
@kayagokalp kayagokalp deleted the kayagokalp/593 branch September 23, 2022 21:16
xgreenx added a commit that referenced this pull request Jan 20, 2023
#594

This change was done at teh same time with moving of this file into another place, so during conflicts resolving we missed it.
xgreenx added a commit that referenced this pull request Jan 20, 2023
…921)

[This change](#594) was done
at the same time as moving this file into another place, so during
conflict resolution, we missed it.

Closes #593
crypto523 pushed a commit to crypto523/fuel-core that referenced this pull request Oct 7, 2024
…#921)

[This change](FuelLabs/fuel-core#594) was done
at the same time as moving this file into another place, so during
conflict resolution, we missed it.

Closes FuelLabs/fuel-core#593
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
4 participants