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

chore: Address constructor now accepts a range of inputs. #3600

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

petertonysmith94
Copy link
Contributor

@petertonysmith94 petertonysmith94 commented Jan 20, 2025

Release notes

In this release, we:

  • The Address constructor now accepts a range of different inputs

Summary

  • The Address constructor now accepts a range of inputs:
    • an Address instance
    • a B256 string
    • an EVM address string
    • a public key

Checklist

  • All changes are covered by tests (or not applicable)
  • All changes are documented (or not applicable)
  • I reviewed the entire PR myself (preferably, on GH UI)
  • I described all Breaking Changes (or there's none)

Copy link

vercel bot commented Jan 20, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
fuels-template ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 22, 2025 4:36pm
ts-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 22, 2025 4:36pm
ts-docs-api ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 22, 2025 4:36pm

@petertonysmith94 petertonysmith94 self-assigned this Jan 20, 2025
@petertonysmith94
Copy link
Contributor Author

Opened this PR early (need to update documentation, etc), just to ensure this is the direction we wish to pursue. Does anyone have any issues with this approach?

cc @FuelLabs/sdk-ts @LuizAsFight

@@ -240,8 +221,7 @@ export class Address {
);
}

const paddedAddress = padFirst12BytesOfEvmAddress(evmAddress);
return new Address(paddedAddress);
return new Address(evmAddress);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand why we'd want to have the Address class represent anything but a fuel-compatible address. It seems more appropriate to me that we differentiate between an Address and an EvmAddress, the same way Sway does it.

@LuizAsFight @luizstacio What are your thoughts here?

Copy link
Contributor

Choose a reason for hiding this comment

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

@nedsalk I believe this is done for simplicity since on Fuel an EVM address is represented the same way as Fuel addresses, using 32 bytes. The difference is that an EVM address is padded with zeros to make it 32 bytes.

Torres-ssf
Torres-ssf previously approved these changes Jan 21, 2025
Copy link
Contributor

@Torres-ssf Torres-ssf left a comment

Choose a reason for hiding this comment

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

Great job @petertonysmith94 🚀

@Torres-ssf Torres-ssf dismissed their stale review January 21, 2025 13:43

Premature approval

Comment on lines 134 to +141
*
* @param publicKey - A wallets public key
* @returns A new `Address` instance
*
* @deprecated Use `new Address` instead
*/
static fromPublicKey(publicKey: string): Address {
if (!isPublicKey(publicKey)) {
throw new FuelError(FuelError.CODES.INVALID_PUBLIC_KEY, `Invalid Public Key: ${publicKey}.`);
}

const b256Address = hexlify(sha256(arrayify(publicKey)));
const b256Address = fromPublicKeyToB256(publicKey);
Copy link
Contributor

Choose a reason for hiding this comment

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

@petertonysmith94 Should we update the docs to conform with these new guidelines?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Torres-ssf as stated in the comment, opened this early to get feedback/confirmation to proceed.

If we're happy with the approach, I will finalise the documentation + update references :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Coverage Report:

Lines Branches Functions Statements
77.08%(+0.04%) 70.67%(+0.03%) 75.14%(+0.04%) 77.08%(+0.04%)
Changed Files:
Ok File (✨=New File) Lines Branches Functions Statements
🔴 packages/account/src/providers/transaction-request/transaction-request.ts 88.57%
(+0%)
76.71%
(-1.37%)
84%
(+0%)
88.81%
(+0%)
🔴 packages/address/src/address.ts 90.69%
(-0.21%)
92.85%
(+6.19%)
85.71%
(+0%)
90.9%
(-0.17%)
🔴 packages/address/src/utils.ts 80.95%
(+11.72%)
68.75%
(+18.75%)
83.33%
(+5.56%)
81.39%
(+11.02%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Issue is a chore
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve Address DX
3 participants