Accept address as String instead of Address

[luizstacio]

When using;

• BaseWalletLocked.<transfer|buildPredicateTransaction |submitPredicate>
• Provider.<getCoins|getResourcesToSpend|getBalance|getBalances|getMessages|buildSpendPredicate|submitSpendPredicate>
• BaseTransactionRequest.<updateWitnessByOwner>

These methods require the Application to instantiate an Address object before executing the methods, as bech32 and hex are both strings and can be differentiated by string evaluation. All the methods should be able also to receive a string. Making address param type from AbstractAddress to something likestring | AbstractAddress. And use Address.fromDynamicInput internally for the correctness of the data.

Note

• Related to Feedback - Frontend #1537

[arboleya]

I would advocate accepting only string instead of string | AbstractAddress.

Still on the same wave about avoiding parameter type overloading and why it sounds like the thing to do.

For this issue, this means that:

1. It is a breaking change
2. It requires refactoring all places currently using the affected methods
   • Mostly purging boilerplate-code anyways
3. It sucks

Still, I think it is worth it.

Thoughts?

cc @FuelLabs/sdk-ts

[luizstacio]

I think is good to accept both once the code you implement the call can have an Address instance and forcing it to transform into a string to then pass the param, would not be so good as just accepting both.

[Dhaiwat10]

@arboleya I would lean towards simplicity, too.

Our methods only accepting string inputs and our code internally taking care of all the complexity around parsing the input & processing it further sounds like a better experience for our users than the methods accepting inputs of multiple shapes and our internal code still having to do some processing depending on the shape of the input anyways.

The former approach sounds like it makes things super simple for the users, while the latter sounds like it involves a compromise on both ends.

[LuizAsFight]

I agree inputs should be more flexible.. string | AbstractAddress sounds good to me

but I remember we kept Address.from.... approach in name of security, but now using Address.fromDynamicInput in a lot of places with only a string input doesn't skip that security thought? sounds weird to me... not related to this issue but I still think new Address(string) should just do the job instead of having Address.from......

[arboleya]

@LuizAsFight It looks like you have a valid point with the fromDynamicInput thing.

---

@luizstacio By having the address string representation saved and always accessible, the idea is that we wouldn't need to convert anything or pass class instances back and forth.

class SampleClass {
  address: string; // always accessible
  constructor(address: string) {
    this.address = address;
    this.abstractAddress = Address.fromDynamicInput(this.address);
  }
}

class OtherClass {
  constructor(address: string) {
    // ...
  }
}

const abc = new SampleClass('0x...');
const xyz = new OtherClass(abc.address); // `abc.address` instead of `abc`

---

@Dhaiwat10 Precisely, thanks for putting it so eloquently. Reducing the number of questions one will ask when reading the code seems favorable, and that's the opposite of what types-overloading causes, IMO.

I suspect that upon seeing address: string, the likely reaction would be:

• Paste the address

Whereas string | AbstractAddress could raise other questions, i.e.:

• What is AbstractAddress?
• Why and when should I use it?

Seeing one thing in isolation might not transmit the complexity that simple decisions can add (or remove), but it can quickly become noticeable when the same habit is repeated throughout the codebase.

[camsjams]

The main issue here is that we want application developers to be fully aware of what kind of address they have. If they don't know what the format is and allow incorrect data, then failures are the least of their worries. One could easily misuse a string in the wrong place and send coins to an incorrect address.

This was originally written to be strict following how the Rust SDK works, where everything is within an Address wrapper.

By forcing developers to use an Address object they are actually less worried about knowing the format, but also internally our code should be very strict about inputs when talking to each package, the end-user can convert a string to an Address and be done with it.

After much thought I'm not too sold on this change making Address data less trustworthy between packages. @digorithm any thoughts here?

[arboleya]

@camsjams All address formats are represented as a string type, and no amount of boilerplate code or manual instantiation will remedy the fact that we have multiple formats by design.

Our code strictness will not change regardless; the detection/validation logic is already in place and can never be removed. Nothing will change internally. We'd be adding an obstacle, not a feature.

It also adds no security. No matter what or how many steps we add, the underlying format will forever be a string. Shifting the remote possibility of mistakes to the left or right, adding one or several steps in between, makes no difference.

The point @LuizAsFight brought up about Address.fromDynamicInput only proves that not even we can't stand this approach, falling back to an umbrella method everywhere to get around it.

No matter the angle I look at it, I continue adamantly about simplicity being the ultimate aim.

[luizstacio]

The idea is that we should accept strings but check for a format bech32. This will keep the security but make it simpler for developers. The utilities around the Address will be provided if the user doesn't have the correct address, it would be possible to convert.

But asking for an Address object makes things more complex and does not add security, as mentioned, when using it, we always do Address.from<...> and pass it along.

In the future, fuel-core should also accept bech32 removing all the need to parse bech32 to hex and vice-versa

[camsjams]

The point @LuizAsFight brought up about Address.fromDynamicInput only proves that not even we can't stand this approach, falling back to an umbrella method everywhere to get around it.

@arboleya I'd agree with that sentiment if we did use Address.fromDynamicInput everywhere, but that is a helper method for application developers. Everywhere inside the code base we use an Address object, we don't use that helper anywhere inside the code (recently @luizstacio added it in for Predicate update)

I only see from methods used in very specific translation methods which would still be needed even if we did switch to back to strings:

Here was the original PR btw:
#441

For this comment @Dhaiwat10 @arboleya

Our methods only accepting string inputs and our code internally taking care of all the complexity around parsing the input & processing it further sounds like a better experience for our users than the methods accepting inputs of multiple shapes and our internal code still having to do some processing depending on the shape of the input anyways.

I'd argue we are already like this former approach - allowing any string vs known object of type Address means we would indeed need to have internal code accept multiple shapes and process depending on input.

We want bad addresses to fail as quickly as possible right?

In current style:

1. User gives bech32
2. parse as Address, now inside well trusted container, fail fast if invalid address format
3. Provider needs b256
4. Using trusted container, convert toB256

In string style my fear is:

1. User gives bech32
2. pass around unknown weakly typed string
3. Provider needs b256
4. Convert unknown string into b256, might fail now because we have invalid format

If we optimized string style:

1. User gives bech32
2. its an unknown string, but we check that its valid, fail fast if invalid address format
3. Provider needs b256
4. Convert unknown string into b256, should not fail because we pre-screened invalid format but not guaranteed

[arboleya]

@camsjams Thanks for the thorough explanation and link to the original PR.

Could you please exemplify these three cases (current style, string style, optimized string style) with simple code snippets from a user's perspective when consuming the SDK? That can be clarifying.

I will also try simulating these cases while reviewing the source code again to understand it better.

[arboleya]

@LuizAsFight Do you remember the places where you mentioned that Address.fromDynamicInput is used? This can also help contextualize this conversation since we're going back and forth between ideals and real-world case scenarios.

[LuizAsFight]

feels bikeshedding such a long discussion because of 1 type more in input... this problem only happens because we don't have a standard way of using this inputs for Address

What I would do: same pattern of math/bn

• create AddressInput

  type AddressInput = Bech32Address | B256Address | string | AbstractAddress;
// could even include `BytesLike` here? I see it's used in `isBech32` function

then Address constructor is the place that validate everything and guarantee a valid/safe Address. for example

  constructor(address: AddressInput) {
    super();
    logger.checkNew(new.target, Address);
    if (typeof address === 'string') {
      if (isB256(address)) {
        this.bech32Address = toBech32(address);
      }

      if (isBech32(address)) {
        this.bech32Address = normalizeBech32(address as Bech32Address);
      }

      if (isPublicKey(address)) {
        this.bech32Address = toBech32(sha256(address));
      }
    } else {
      this.bech32Address = address.toAddress();
    }
    
    if (!this.bech32Address || !isBech32(this.bech32Address)) {
      logger.throwArgumentError('Invalid Address', 'address', address);
    }
  }

---

On this way you don't have problems or decisions to do regarding inputs

anywhere you need address input you will use:

export function test(address?: AddressInput) {
  const created = new Address(address);

  // from here on you know your `created` variable is a valid address.
}

or if you don't wanna allow flexible inputs, but only string... would work on exact same way:

export function test(address?: string) {
  const created = new Address(address);

  // from here on you know your `created` variable is a valid address.
}

plus

Using different from... to create an Address introduces split rules, opening room for problems. for example RN only looking to fromString code

  static fromString(address: string): Address {
    return isBech32(address) ? new Address(address as Bech32Address) : this.fromB256(address);
  }

it can be broken because it suppose when it's not a Bech32, it's a b256 ?? I didn't test but looks like.

but this will only happen if you use fromString... if you use fromDynamicInput then there're other rules.
if we do like I mentioned before, you centralize responsibility for a valid Address. I don't like the idea of having static functions with part of the responsibility to have a valid address output.

[LuizAsFight]

@LuizAsFight Do you remember the places where you mentioned that Address.fromDynamicInput is used? This can also help contextualize this conversation since we're going back and forth between ideals and real-world case scenarios.

@arboleya it's a new thing I guess it's not used everywhere, sorry if I meant it. My comment was just pointing it out because this issue mention using Address.fromDynamicInput

[luizstacio]

I open a draft PR for better understand the issue, and open it for discussion #801

[arboleya]

Completed via #1583.