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

feat: Lotus Send CLI: Lotus send should work with ETH address receipients #12319

Merged
merged 5 commits into from
Jul 30, 2024

Conversation

aarshkshah1992
Copy link
Contributor

@aarshkshah1992 aarshkshah1992 commented Jul 29, 2024

This fixes the Lotus send CLI to work with ETH address recipients (both 0xff... corresponding to Filecoin f0 addresses and 0xabcd corresponding to Filecoin f410f addresses).

Have tested this locally on a local devnet.

./lotus send --from-eth-addr 0x4582e0a8f02f45dfb6cca081b1cb82fcf0508c34 0xff000000000000000000000000000000000003ed 10

f4 addr:  t410fiwbobkhqf5c57nwmuca3ds4c7tyfbdbun3hxwmi
Sending message from: t410fiwbobkhqf5c57nwmuca3ds4c7tyfbdbun3hxwmi
Sending message to: t01005
Using Method: 3844450837
bafy2bzacec3lcrfevp4wsuya65t2nien2cjselbmqd3cnr2w32hukbwxg57zy
./lotus send 0xff000000000000000000000000000000000003ed 10
Sending message from: t3uhfeeespjbuhde6vfxvgyuhoj4danwv3fdsk3m3t46p2kpegm76jpvukgnlb2w7aucldnmq2j4un3nlxs73a
Sending message to: t01005
Using Method: 0
bafy2bzacedwvhrfdopwr7azpjcjvjw22orirulikewdpw6zgc3tg5bgspv3vu
./lotus send 0x4582e0a8f02f45dfb6cca081b1cb82fcf0508c34 100
Sending message from: t3uhfeeespjbuhde6vfxvgyuhoj4danwv3fdsk3m3t46p2kpegm76jpvukgnlb2w7aucldnmq2j4un3nlxs73a
Sending message to: t410fiwbobkhqf5c57nwmuca3ds4c7tyfbdbun3hxwmi
Using Method: 0
bafy2bzacebnj45yttbtcd75rajfwkqpc27tsru6brfhufolnjtvhsixltitqo
./lotus send --from-eth-addr 0x4582e0a8f02f45dfb6cca081b1cb82fcf0508c34 0xff000000000000000000000000000000000003ec  0.2
f4 addr:  t410fiwbobkhqf5c57nwmuca3ds4c7tyfbdbun3hxwmi
Sending message from: t410fiwbobkhqf5c57nwmuca3ds4c7tyfbdbun3hxwmi
Sending message to: t01004
Using Method: 3844450837
bafy2bzacedk25ku4sny272z6erhha4bx5pymbfued4wu6434z4yoaka2n6rvu
./lotus send --from-eth-addr 0x4582e0a8f02f45dfb6cca081b1cb82fcf0508c34 0x28dfd8668534e9caa8fb0b77577c8b78831d0d07 0.2
f4 addr:  t410fiwbobkhqf5c57nwmuca3ds4c7tyfbdbun3hxwmi
Sending message from: t410fiwbobkhqf5c57nwmuca3ds4c7tyfbdbun3hxwmi
Sending message to: t410ffdp5qzufgtu4vkh3bn3vo7elpcbr2dih4tfslca
Using Method: 3844450837
bafy2bzacedrwb75aagtwl74krto7aym5ato7vwqtg6cc4wlgzvmf3wiwrp7ik
 ./lotus send --from-eth-addr 0x4582e0a8f02f45dfb6cca081b1cb82fcf0508c34 t01005 10
f4 addr:  t410fiwbobkhqf5c57nwmuca3ds4c7tyfbdbun3hxwmi
Sending message from: t410fiwbobkhqf5c57nwmuca3ds4c7tyfbdbun3hxwmi
Sending message to: t01005
Using Method: 3844450837
bafy2bzaceaijk7lflnkt3l6s6irnxsmmoennfj57jpjgn3zi4e53t6qbokkae
./lotus send --from t410ffdp5qzufgtu4vkh3bn3vo7elpcbr2dih4tfslca t01005 0.2
Sending message from: t410ffdp5qzufgtu4vkh3bn3vo7elpcbr2dih4tfslca
Sending message to: t01005
Using Method: 3844450837
bafy2bzaceb5o4uqbktypwpiabxmqycxku6lcz5yace4hxkvlkhyd424shpdhg
./lotus send --from t410ffdp5qzufgtu4vkh3bn3vo7elpcbr2dih4tfslca 0xff000000000000000000000000000000000003ed  0.1
Sending message from: t410ffdp5qzufgtu4vkh3bn3vo7elpcbr2dih4tfslca
Sending message to: t01005
Using Method: 3844450837
bafy2bzacecqggccw43b455eady3fdv2apk7tybfi5ua2qrcaox33eo3la4yna

@aarshkshah1992
Copy link
Contributor Author

@rvagg Also tested this with a smart contract as a receipient (using both ETH and non-ETH sender) and that also works fine.

cli/send.go Show resolved Hide resolved
cli/send.go Outdated Show resolved Hide resolved
@rvagg
Copy link
Member

rvagg commented Jul 30, 2024

Seems pretty straightforward; in terms of enabling what we want to work properly at least.

You followed up with each of those sends and confirmed that the recipient received it? Is this something we could capture in a simple itest, not necessarily using the CLI but using the send logic for the various cases.

aarshkshah1992 and others added 2 commits July 30, 2024 11:52
Co-authored-by: Rod Vagg <rod@vagg.org>
@aarshkshah1992
Copy link
Contributor Author

@rvagg

  • Fixed test and accepted your change
  • Yes for each of those cases I verified that the recipient did indeed get the funds.
  • Writing automated tests for each of those cases will probably have to use itests that construct the same message that Lotus send constructs here and then submitting them to the mpool and asserting balances. But that is bigger than the change I'm making here.

@aarshkshah1992 aarshkshah1992 merged commit e75c5ff into master Jul 30, 2024
84 checks passed
@aarshkshah1992 aarshkshah1992 deleted the feat/lotus-send-eth-recipient-support branch July 30, 2024 11:37
@aarshkshah1992 aarshkshah1992 restored the feat/lotus-send-eth-recipient-support branch July 30, 2024 11:37
@aarshkshah1992 aarshkshah1992 deleted the feat/lotus-send-eth-recipient-support branch July 30, 2024 11:37
@@ -128,7 +147,7 @@ var SendCmd = &cli.Command{
params.Params = decparams
}

if ethtypes.IsEthAddress(params.From) {
if ethtypes.IsEthAddress(params.From) || ethtypes.IsEthAddress(params.To) {
Copy link
Member

Choose a reason for hiding this comment

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

I think this is going to cause some issues.

  1. There are almost certainly users using native accounts to send messages to EVM smart contracts, explicitly specifying the method number.
  2. Implicitly calling InvokeContract is the nice thing to do, but that's a significant change of behavior.

Ideally this would have this method number handling if and only if the user passes a literal 0x... address in the to field. If they pass a normal Filecoin address, they should get the normal Filecoin method handling.

Copy link
Member

Choose a reason for hiding this comment

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

pulled this up into a new issue #12326

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

Successfully merging this pull request may close these issues.

3 participants