feat: add Into for Address and ContractId fn arguments

[hal3e]

closes: #962

Our FunctionGenerator now generates impl Into<Bech32Address> and impl Into<Bech32ContractId> instead of Address and ContractId.

This is reflected on all contract methods, predicate encode functions, and script main functions.

Checklist

• I have linked to any relevant issues.
• I have added necessary labels.
• I have done my best to ensure that my PR adheres to the Fuel Labs Code Review Standards.
• I have requested a review from the relevant team or maintainers.

[iqdecay]

A question on design

[iqdecay]

I feel like this would enable the user to pass in an Address to a method that normally only accepts ContractId without being explicit about it, and this is not something we want. It needs to be explicit, because the difference between Address and ContractId is fundamental and can lead to a loss of fund IIUC. Am I getting this wrong?

[hal3e]

I feel like this would enable the user to pass in an Address to a method that normally only accepts ContractId without being explicit about it, and this is not something we want. It needs to be explicit, because the difference between Address and ContractId is fundamental and can lead to a loss of fund IIUC. Am I getting this wrong?

You are right. Normally it would not be a problem to have a impl Into<ContractId> as I would imagine that into would be implemented only for types that make sense. However, we will get into implementations for conversion between Address and ContractId and then we do not have type safety anymore.

The only other thing that comes to mind is to have methods accept Bech32Address and Bech32ContractId. However, this solution is not as ergonomic as impl Into<ContractId> as you would have to turn an already existing ContractID into Bech32ContractId (same applies to Address)

@FuelLabs/sdk-rust what are you thoughts on this? I am leaning towards leaving it as is and closing this PR.

[segfault-magnet]

I feel like this would enable the user to pass in an Address to a method that normally only accepts ContractId without being explicit about it, and this is not something we want. It needs to be explicit, because the difference between Address and ContractId is fundamental and can lead to a loss of fund IIUC. Am I getting this wrong?

You are right. Normally it would not be a problem to have a impl Into<ContractId> as I would imagine that into would be implemented only for types that make sense. However, we will get into implementations for conversion between Address and ContractId and then we do not have type safety anymore.

The only other thing that comes to mind is to have methods accept Bech32Address and Bech32ContractId. However, this solution is not as ergonomic as impl Into<ContractId> as you would have to turn an already existing ContractID into Bech32ContractId (same applies to Address)

@FuelLabs/sdk-rust what are you thoughts on this? I am leaning towards leaving it as is and closing this PR.

What about replacing the usages of Address with impl Into<Bech32Address> in the generated fn bindings?

So you can then give both an Address and its bech32 equivalent. Same for the ContractIds.

You'd retain type safety since neither Address satisfies impl Into<Bech32ContractId> nor does ContractId satisfy impl Into<Bech32Address>.

You'd still have pure addresses nested in generated structs and enums, since using impl there would cause generic headaches.

[iqdecay]

I would like the Into<Bech32Address> and Into<Bech32ContractId>, but sending something to a Bech32Address vs its corresponding Address doesn't work I think. If they are not functionaly equivalent, I'm for keeping type safety above all, otherwise users would probably shoot themselves in the foot.
But it does ask the question of why we are bothering users with implementation details of exposing the Bech32 format at all, IMO we should discuss around how to abstract this away.

[hal3e]

What about replacing the usages of Address with impl Into<Bech32Address> in the generated fn bindings?

So you can then give both an Address and its bech32 equivalent. Same for the ContractIds.

You'd retain type safety since neither Address satisfies impl Into<Bech32ContractId> nor does ContractId satisfy impl Into<Bech32Address>.

You'd still have pure addresses nested in generated structs and enums, since using impl there would cause generic headaches.

This is a great idea! ❤️ I did a quick first implementation and it works as expected. We retain the type safety as you said.

@iqdecay you said:

but sending something to a Bech32Address vs its corresponding Address doesn't work I think.

We turn it back to an Address under the hood so everything works the same. The interface is just a bit nicer as you can give either an Address or a Bech32Address

[iqdecay]

What I meant is I think that rather than changing this implementation detail, we should re-explore why we are exposing two types of addresses at all. Fixing this seems like a bandaid to the more general problem of exposing two types of addresses instead of handling them both under the hood. This particular fix is moreover not the one requested by user (because the one requested by users is a gateway to a footgun), so we don't actuallly need to do it IMO.

[hal3e]

What I meant is I think that rather than changing this implementation detail, we should re-explore why we are exposing two types of addresses at all. Fixing this seems like a bandaid to the more general problem of exposing two types of addresses instead of handling them both under the hood. This particular fix is moreover not the one requested by user (because the one requested by users is a gateway to a footgun), so we don't actuallly need to do it IMO.

I agree but what you say is above this request. The user wanted a nice UI where he can give either Address or Bech32Address (same for ContractId) to contract methods. This PR makes that possible without losing type safety. So until we have an unified Address and ContractId IMO it makes sense to have this.

[iqdecay]

Ah yes you're right mb

[iqdecay]

I'm sorry but I really don't want to merge anything before the fuel-core 0.18 is merged

[segfault-magnet]

Good stuff.

[Salka1988]

👍

[iqdecay]

nice!

[SwayStar123]

Thank you hal3e, very cool

[diyahir]

Could we not determine if it's a contract or address at run-time and avoid casting it altogether?

Casting it only at runtime for the pieces that must know

[diyahir]

If I give you an address 0x123....789, and I say to send it 10 ETH, is it fair to say you could get an error depending on if it's a contractId or an address, so you would need to ask me a follow-up, is it an address or a contract. Well a multisig is a contract, but I could see someone making that mistake.

It seems like we're building a 'gotcha' into the sway stack that could be avoided entirely if we just abstract all of this into a run-time check (if possible, if not possible currently then add a flag to each address?)

[iqdecay]

This is a good argument I agree, but this is not the right place to discuss it, this issue is. This design decision is not ours, and even if we wanted we cannot change it, it's in Sway.