filecoin-project / lotus

Reference implementation of the Filecoin protocol, written in Go
https://lotus.filecoin.io/
Other
2.84k stars 1.26k forks source link

Lotus Wallet and Mpool RPC should accept "0x" recipients for messages and execute the message using the correct method #12331

Open aarshkshah1992 opened 2 months ago

aarshkshah1992 commented 2 months ago

Checklist

Lotus component

What is the motivation behind this feature request? Is your feature request related to a problem? Please describe.

We have the following two APIs in Lotus that clients can use to sign and submit Filecoin messages.

// WalletSignMessage signs the given message using the given address.
WalletSignMessage(context.Context, address.Address, *types.Message) (*types.SignedMessage, error) //perm:sign

followed by

MpoolPush(context.Context, *types.SignedMessage) (cid.Cid, error) //perm:write
MpoolPushMessage(ctx context.Context, msg *types.Message, spec *MessageSendSpec) (*types.SignedMessage, error) //perm:sign

However, none of these APIs allow the recipient of the message to be a 0x address.

This is an important feature to have if you want to enable exchanges to allow withdrawals to 0x addresses without the exchanges having to jump through multiple hoops on their side.

Once the above is in place, exchanges can use these APIs directly even for messages where the recipient is a 0x address. These APIs can then internally convert the 0x address to the correct Filecoin address (0xff.. -> f0.. and 0xabcd -> f4q0f...) and also use the correct method in the message (Always use InvokeEVM if the user passes a 0x address).

Note that this will also need work in go-address to support parsing of 0x addresses in the RPC request to a Filecoin addresses.

Questions

1) Can we get away with ALWAYS using the InvokeEVM method if the recipient is a f410f address ? This could be if the client passes a f410f address OR a 0xabcd address (which gets converted to an f410f address). The problem here is that once go-address converts a 0xabcd address to an f410f address -> we don't know what the client passed.

2) Is it okay if we retain the user's existing method for f0... addresses ? (Even if the client passed in a 0xff address) ?

rvagg commented 2 months ago

ISTM that changing WalletSignMessage would involve one of two options:

  1. Rewriting the provided types.Message if it has an 0x address by converting it to a proper filecoin address; i.e. "I'll sign this for you, but first I'm going to change what you've given me, so I'll actually sign something else"
  2. Changing the spec (and consensus, so a FIP, if this is even possible) to allow 0x addresses in types.Message, so we don't have to molest what's provided

As for changing MpoolPush, I think the only option is number 2 above, it's signed, we can't modify it, you'd have to be able to submit a raw, signed message (bytes) to the mpool that has a 0x in it.

rvagg commented 2 months ago

Oh, and types.Message doesn't actually have space for a 0x, so changing WalletSignMessage would mean either making it accept a different type, or changing types.Message in some way that is either consensus breaking, or make it have a weirdly flexible address type that is only a proper filecoin address when in the message pool or in blocks but can be something else when serialised for RPC calls like this.

So I think you're looking squarely at consensus breaking and introducing 0x into the formal protocol to make this work nicely.

Which I'm not sure is better than just telling exchanges to get the proper filecoin address before submitting a message.

aarshkshah1992 commented 2 months ago

@rvagg

Wait. Can't we change the serialization/deserialization logic in go-address so that a 0x address passed by the client in the request JSON to WalletSignMessage etc gets converted to the corresponding f0/f4 address when the Message Json is unmarshalled to a types.Message for the RPC call ?

rvagg commented 2 months ago

that's option 1 - we're going to sign something other than what you asked us to sign

Stebalien commented 2 months ago

I believe this will cause more problems than it fixes:

  1. It's very "magical" in a part of the API that's supposed to be very well defined and non-magical.
  2. As far as I know, very few exchanges are using WalletSignMessage (for very good security reasons...).

I agree this is a problem, but I think the correct solution is to provide better tooling and work with exchanges.


For example:

Can we get away with ALWAYS using the InvokeEVM method if the recipient is a f410f address ?

Yes, we can. However, that means we'll potentially be invoking user code even when the user specifies method-0, which could cost an arbitrary amount of gas. Ideally users are enforcing gas limits, but the expectation is that method-0 sends never perform arbitrary user operations.

Wait. Can't we change the serialization/deserialization logic in go-address so that a 0x address passed by the client in the request JSON to WalletSignMessage etc gets converted to the corresponding f0/f4 address when the Message Json is unmarshalled to a types.Message for the RPC call ?

We can, but, then we wouldn't be able to distinguish between the user specifying 0x... and f410f.... And if the user specifies f410f... we absolutely shouldn't change the method number.


One potential option here is to:

  1. Introduce a new APIMessage type.
  2. Make the To field special (accept 0x and f addresses).
  3. Make the Method field optional. If unspecified, we use the default method for the target address.

This would "do the thing" and force users to opt-in to the "do the thing" behavior.

Even better, we might just want to introduce an entirely new SendFunds method that behaves very much like lotus send.

Regardless, I'm still skeptical this is going to help as I expect very few Exchanges are trusting lotus to manage their keys.

aarshkshah1992 commented 2 months ago

I think the best option here is for the signing libs that exchanges use to upgrade to allow conversion of "0x" addresses to "f4/f0" addresses and produce a message with the "correct method" (something similar to what lotus send does now).

I agree that changing this on Lotus does not make sense given all the magic that needs to be introduced in the APIs. Also, introducing a new message type/API here does not make sense as instead of exchanges doing the work of using the new API -> they can simply convert the "0x" address to "f**" address before signing the message using our existing tooling. This issue was motivated by exchange inertia in changing their existing flow.

Stebalien commented 2 months ago

Yep, that was my conclusion as well.

jennijuju commented 2 weeks ago

@aarshkshah1992 @Stebalien should we label this as wont fix and close the issue?