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

[Support/Filecoin] Add ERC20 tokens support for Filecoin #6704

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

lawRathod
Copy link
Contributor

βœ… Checklist

  • npx changeset was attached.
  • Covered by automatic tests.
  • Impact of the changes:
    • ...

πŸ“ Description

Add ERC20 token support for filecoin network. Adds new tokens specific to filecoin and enable send/receive.

❓ Context

  • JIRA or GitHub link:

🧐 Checklist for the PR Reviewers

  • The code aligns with the requirements described in the linked JIRA or GitHub issue.
  • The PR description clearly documents the changes made and explains any technical trade-offs or design decisions.
  • There are no undocumented trade-offs, technical debt, or maintainability issues.
  • The PR has been tested thoroughly, and any potential edge cases have been considered and handled.
  • Any new dependencies have been justified and documented.
  • Performance considerations have been taken into account. (changes have been profiled or benchmarked if necessary)

Copy link

vercel bot commented Apr 17, 2024

The latest updates on your projects. Learn more about Vercel for Git β†—οΈŽ

Name Status Preview Comments Updated (UTC)
web-tools ❌ Failed (Inspect) Jun 4, 2024 11:21am
3 Ignored Deployments
Name Status Preview Comments Updated (UTC)
ledger-live-docs ⬜️ Ignored (Inspect) Visit Preview Jun 4, 2024 11:21am
native-ui-storybook ⬜️ Ignored (Inspect) Visit Preview Jun 4, 2024 11:21am
react-ui-storybook ⬜️ Ignored (Inspect) Visit Preview Jun 4, 2024 11:21am

Copy link

vercel bot commented Apr 17, 2024

@lawRathod is attempting to deploy a commit to the LedgerHQ Team on Vercel.

A member of the Team first needs to authorize it.

@live-github-bot live-github-bot bot added desktop Has changes in LLD mobile Has changes in LLM common Has changes in live-common ledgerjs Has changes in the ledgerjs open source libs translations Translation files have been touched fork Pull request base branch comes from a fork. labels Apr 17, 2024
@lawRathod lawRathod force-pushed the support/filecoin-evm branch 2 times, most recently from df6c770 to ceae9ec Compare April 18, 2024 09:14
@lawRathod lawRathod marked this pull request as ready for review April 19, 2024 14:06
@lawRathod lawRathod requested a review from a team as a code owner April 19, 2024 14:06
Copy link

There as been no activity on this PR for the last 14 days. Please consider closing this PR.

@github-actions github-actions bot added the Stale label May 22, 2024

const accountType = addr.substring(0, 2);
if (["f0", "0x", "f4"].includes(accountType)) {
return true;
Copy link
Contributor

Choose a reason for hiding this comment

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

if I understand correctly, an address begins with "f0" or "f4", we should convert them into eth address and check whether it is a valid one. right?
we can't consider all the addresses begin with "f0" or "f4" are valid. right?

Copy link
Contributor

Choose a reason for hiding this comment

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

what if an address begins with "f1", is it valid?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you understand correct, for token transfer we need the address to be eth style (0x) not f[0,1,4]. In filecoin ecosystem there's ways to convert all native addresses to eth style but f1 is exception which requires network call (insecure) to get equivalent f0 and then f0 -> 0x (secure).

For addresses beginning with f1 we do not want to support them for token transfer because network call can be insecure in practice and lead to possible known attacks. For when invalid we therefore have this error.

@@ -240,6 +242,7 @@
"tsx": "^4.7.1",
"usehooks-ts": "^2.13.0",
"utility-types": "^3.10.0",
"web3": "^4.7.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

we use
the "ethers" lib instead of "web3" in our codebase.

i.e.
https://github.com/LedgerHQ/ledger-live/blob/develop/libs/coin-modules/coin-evm/package.json#L75

could you plz change it to "ethers" lib to be coherent with the rest of our codebase? Thanks a lot

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely, now using ethers instead of web3.

@@ -131,6 +131,7 @@
"@elrondnetwork/erdjs": "11.0.0",
"@elrondnetwork/erdjs-network-providers": "^1.1.2",
"@hashgraph/sdk": "2.14.2",
"@ipld/dag-cbor": "^9.2.0",
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 find it is used in the current code. Isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's definitely not, now removed from package.json... it was an accident πŸ˜…

const buffer = Buffer.from(abiEncodedParams.slice(2), "hex"); // buffer/byte array
const dataEncoded = cbor.encode(buffer);

return base64.stringify(dataEncoded);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can use
Buffer.from
and
buffer.toString('base64');

instead, so that we don't have to use "rfc4648" lib

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great tip, I didn't know it could be done this way. Now using this way and the rfc4648 is removed from the package.json. πŸ˜„

@@ -5495,6 +5495,9 @@
"title": "Impossible to calculate amount and fees",
"description": "Impossible to calculate amount and fees"
},
"InvalidRecipientForTokenTransfer": {
Copy link
Contributor

Choose a reason for hiding this comment

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

should we do the same translation for Ledger Mobile?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes definitely, I am sorry it wasn't there but now I have added it.

const bnBalance = new BigNumber(balance.toString());
const tokenAccountId = encodeTokenAccountId(parentAccountId, token);

const operations = txns
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a question: For the subaccount operations, are all the addresses retrieved from the explorer endpoint expressed in the Ethereum format instead of the Filecoin format?

If a user clicks the "receive" button, does he get an Ethereum format address or a Filecoin format address?

Do you think it is better to convert back to the Filecoin format in the UI? If a user sends ERC20 tokens using his Filecoin account in the UI, I think he expects to see his Filecoin address as the sender in the operation list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the subaccount operations, are all the addresses retrieved from the explorer endpoint expressed in the Ethereum format instead of the Filecoin format?

Yes they are, the erc20 endpoints on indexing only stores the address data from the evm world. The translation isn't yet there.

If a user clicks the "receive" button, does he get an Ethereum format address or a Filecoin format address?

The user will see a filecoin address format with f1 protocol. Converting to f1 -> 0x involves a lookup from node through network call which we think might cause trouble hence the translation isn't supported. More info here

Do you think it is better to convert back to the Filecoin format in the UI?

It's definitely better user experience IMO. It will definitely cost more network calls because going from 0x to f1 might be tricky, the other way is much easier. We already do convert f1 -> 0x to check if the account is sending or receiving, I think it would be okay to override the sender/reciever address at least when creating operations. What do you think?

Copy link
Contributor

@hzheng-ledger hzheng-ledger left a comment

Choose a reason for hiding this comment

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

Please have a look at my comments. Thanks a lot

@lawRathod
Copy link
Contributor Author

Please have a look at my comments. Thanks a lot

Thanks for the review πŸ™ŒπŸ», I tried to work on the comments. Please have another look when possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
common Has changes in live-common desktop Has changes in LLD fork Pull request base branch comes from a fork. ledgerjs Has changes in the ledgerjs open source libs mobile Has changes in LLM translations Translation files have been touched ui Has changes in the design system library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants