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

(Wallet) Enable Filecoin support flag #18881

Merged
merged 17 commits into from
Jun 23, 2023
Merged

Conversation

simoarpe
Copy link
Collaborator

@simoarpe simoarpe commented Jun 13, 2023

Resolves brave/brave-browser#30342

Resolves brave/brave-browser#30402

Security review issue https://github.com/brave/security/issues/1321

Submitter Checklist:

  • I confirm that no security/privacy review is needed, or that I have requested one
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally:
    • npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
    • npm run lint, npm run presubmit wiki, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

  • Navigate to the Brave Wallet and create a new Filecoin testnet account
  • Add Filecoins using a faucet service
  • Send a Filecoin amount to a valid Filecoin testnet address
  • Verify the unapproved transaction is created
  • Approve the transaction
  • Navigate to the Accounts section
  • Tap on the Filecoin testnet account
  • Lookup for the transaction and tap on it
  • Verify the link to the blockchain explorer opens correctly

@simoarpe simoarpe added CI/skip-macos-x64 Do not run CI builds for macOS x64 CI/skip-ios Do not run CI builds for iOS CI/skip-windows-x86 feature/web3/wallet CI/skip-windows-x64 Do not run CI builds for Windows x64 unused-CI/skip-linux-x64 Do not run CI builds for Linux x64 labels Jun 13, 2023
@simoarpe simoarpe requested a review from a team as a code owner June 13, 2023 11:35
@simoarpe simoarpe self-assigned this Jun 13, 2023
@supermassive supermassive self-requested a review June 13, 2023 14:32
@simoarpe simoarpe force-pushed the simone/enable-filecoin-support branch from 77e1ac2 to 1f52bab Compare June 13, 2023 14:57
@simoarpe simoarpe marked this pull request as draft June 13, 2023 18:53
@simoarpe simoarpe changed the title Enable Filecoin support flag (Wallet) Enable Filecoin support flag Jun 13, 2023
@simoarpe simoarpe force-pushed the simone/enable-filecoin-support branch from 0c73a7d to 6f6cd36 Compare June 16, 2023 13:58
@simoarpe simoarpe force-pushed the simone/enable-filecoin-support branch from dca6dfc to c9adf52 Compare June 19, 2023 12:09
@simoarpe simoarpe marked this pull request as ready for review June 19, 2023 15:17
Copy link
Member

@SergeyZhukovsky SergeyZhukovsky left a comment

Choose a reason for hiding this comment

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

++

Copy link

@Pavneet-Sing Pavneet-Sing left a comment

Choose a reason for hiding this comment

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

File coin test net send has issue with network, asset, and balance. Desktop has no asset for filecoin test net send, the filecoin test case works?

file-coin-testnet.webm

@simoarpe
Copy link
Collaborator Author

File coin test net send has issue with network, asset, and balance. Desktop has no asset for filecoin test net send, the filecoin test case works?

Thanks, on it!

} else {
String result;
try {
BigInteger minued = new BigInteger(maxFeePerGasDecimal);
Copy link
Collaborator

Choose a reason for hiding this comment

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

minuend

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed, thanks!

@supermassive supermassive requested a review from cypt4 June 20, 2023 10:07
@simoarpe
Copy link
Collaborator Author

@Pavneet-Sing, the issue was related to the logic used to distinguish between network types. Improved the check to distinguish between mainnet and testnet for Filecoins in the commit 4b8c587 (#18881)

Re-requesting a review, thanks!

@simoarpe simoarpe requested a review from Pavneet-Sing June 20, 2023 11:48
@@ -287,29 +287,10 @@ private void addAccountInternal(@CoinType.EnumType int coinType,
result -> { handleAddAccountResult(result, callback); });
}

private static @KeyringId.EnumType int getTargetKeyringId(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Method removed as duplicate of AssetUtils.getKeyring.

@simoarpe simoarpe merged commit da679e8 into master Jun 23, 2023
@simoarpe simoarpe deleted the simone/enable-filecoin-support branch June 23, 2023 20:06
@github-actions github-actions bot added this to the 1.55.x - Nightly milestone Jun 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/skip-ios Do not run CI builds for iOS CI/skip-macos-x64 Do not run CI builds for macOS x64 CI/skip-windows-x64 Do not run CI builds for Windows x64 feature/web3/wallet/core feature/web3/wallet needs-security-review unused-CI/skip-linux-x64 Do not run CI builds for Linux x64
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(Wallet) Implement Filecoin send action (Wallet) Turn on flag to enable Filecoin support
4 participants