chore(wallet): Migrate Report Tx Sent P3A question into core #22360
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
CI/storybook-url
|
A Storybook has been deployed to preview UI for the latest push |
|
A Storybook has been deployed to preview UI for the latest push |
…ation, unit test network P3A switch.
StephenHeaps
force-pushed
the
wallet/p3a-report-transaction-core
branch
from
7c84fb1
to
717dc64
Compare
|
A Storybook has been deployed to preview UI for the latest push |
StephenHeaps
force-pushed
the
wallet/p3a-report-transaction-core
branch
from
717dc64
to
f099e07
Compare
StephenHeaps
force-pushed
the
wallet/p3a-report-transaction-core
branch
from
f099e07
to
8b7cb6a
Compare
|
A Storybook has been deployed to preview UI for the latest push |
supermassive
reviewed
Mar 4, 2024
| } | ||
| auto tx_coin = GetCoinTypeFromTxDataUnion(*tx_info->tx_data_union); | ||
| if (tx_coin == mojom::CoinType::BTC || tx_coin == mojom::CoinType::ZEC) { | ||
| // BTC, ZEV transactions are not tracked yet |
supermassive
reviewed
Mar 4, 2024
| @@ -1967,7 +1977,8 @@ TEST_F(KeyringServiceUnitTest, RestoreLegacyBraveWallet) { | |||
| KeyringService service(json_rpc_service(), GetPrefs(), GetLocalState()); | |||
| BraveWalletService brave_wallet_service( | |||
| shared_url_loader_factory(), nullptr, &service, json_rpc_service(), | |||
| nullptr, nullptr, nullptr, GetPrefs(), GetLocalState(), false); | |||
| tx_service(&service).get(), nullptr, nullptr, GetPrefs(), GetLocalState(), | |||
There was a problem hiding this comment.
This creates unique_ptr<TxService> and immediately destroys it leaving danglig pointer in brave_wallet_service.tx_service_ -- most likely will crash in ASAN builds. It should be either TxService living on stack like KeyringService service above or allowing tx_service to be null for tests(see CHECK_IS_TEST())
There was a problem hiding this comment.
TIL about CHECK_IS_TEST 👍 . I went with MakeTxService route given check_is_test.h documentation.
supermassive
reviewed
Mar 4, 2024
| @@ -170,6 +171,15 @@ class KeyringServiceUnitTest : public testing::Test { | |||
|
|
|||
| JsonRpcService* json_rpc_service() { return json_rpc_service_; } | |||
|
|
|||
| std::unique_ptr<TxService> tx_service(KeyringService* service) { | |||
There was a problem hiding this comment.
tx_service() naming style is for trivial getters only. For this case it should be like MakeTxService(...)
supermassive
reviewed
Mar 4, 2024
| scoped_refptr<network::SharedURLLoaderFactory> shared_url_loader_factory_; | ||
| network::TestURLLoaderFactory url_loader_factory_; | ||
| data_decoder::test::InProcessDataDecoder in_process_data_decoder_; | ||
| std::string tx_hash1_; |
There was a problem hiding this comment.
It seems we don't need tx_hash1_ being a field
…ervice` to avoid dangling pointer & cleanup.
|
A Storybook has been deployed to preview UI for the latest push |
supermassive
reviewed
Mar 5, 2024
| @@ -590,7 +589,7 @@ TEST_F(BraveWalletP3AUnitTest, SolTransactionSentObservation) { | |||
| mojom::TxDataUnion::NewSolanaTxData(std::move(solana_tx_data)), | |||
| mojom::kSolanaMainnet, sol_from(), &tx_meta_id)); | |||
|
|
|||
| tx_hash1_ = | |||
| std::string tx_hash1_ = | |||
There was a problem hiding this comment.
Nit: local variables should not have trailing underscore
supermassive
approved these changes
Mar 5, 2024
|
A Storybook has been deployed to preview UI for the latest push |
DJAndries
approved these changes
Mar 5, 2024
yrliou
approved these changes
Mar 5, 2024
There was a problem hiding this comment.
Just unblocking for wallet-core since @supermassive already approved.
josheleonard
approved these changes
Mar 6, 2024
simoarpe
approved these changes
Mar 6, 2024
nuo-xu
approved these changes
Mar 7, 2024
7 tasks
StephenHeaps
added a commit
that referenced
this pull request
Mar 8, 2024
* Move P3A transaction sent reporting into core
|
Verification PASSED on
EthAccounts Case 1: Brave.Wallet.EthTransactionSent = 0_PASSEDCase 1.1: No Eth TransactionSent
Case 1.2: Eth transactions sent exceeded 7 days_PASSED
Case 2: Brave.Wallet.EthTransactionSent = 1_PASSEDCase 2.1: Eth TransactionSent
Case 2.2: Eth Transaction Sent in past 7 days_PASSED
Solana accounts: Case 1: Brave.Wallet.SolTransactionSent = 0_PASSEDCase 1.1: No Sol TransactionSent
Case 1.2: Sol transactions sent exceeded 7 days_PASSED
Case 2: Brave.Wallet.SolTransactionSent = 1_PASSEDCase 2.1: Sol TransactionSent
Case 2.2: Sol Transaction Sent in past 7 days_PASSED
FilecoinAccounts Case 1: Brave.Wallet.FilTransactionSent = 0_PASSEDCase 1.1: No Fil TransactionSent
Case 1.2: Sol transactions sent exceeded 7 days_PASSED
Case 2: Brave.Wallet.FilTransactionSent = 1_PASSEDCase 2.1: Fil TransactionSent
Case 2.2: Sol Transaction Sent in past 7 days_PASSED
Verification PASSED on
Note: Filecoin is NOT implemented for Android, confirmed with wallet QA spoc @srirambv. Hence not verified Filecoin accounts verification on Android EthAccounts Note: Step 2, 3 are not required on Android as Test nework is enabled by default when brave is launched using Case 1: Brave.Wallet.EthTransactionSent = 0_PASSEDCase 1.1: No Eth TransactionSent
Case 1.2: Eth transactions sent exceeded 7 days_PASSED
Case 2: Brave.Wallet.EthTransactionSent = 1_PASSEDCase 2.1: Eth TransactionSent
Case 2.2: Eth Transaction Sent in past 7 days_PASSED
Solana accounts: Note: Step 2 is not required on Android as Test nework is enabled by default when brave is launched using Case 1: Brave.Wallet.SolTransactionSent = 0_PASSEDCase 1.1: No Sol TransactionSent
Case 1.2: Sol transactions sent exceeded 7 days_PASSED
Case 2: Brave.Wallet.SolTransactionSent = 1_PASSEDCase 2.1: Sol TransactionSent
Case 2.2: Sol Transaction Sent in past 7 days_PASSED
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves brave/brave-browser#36156
Submitter Checklist:
QA/YesorQA/No;release-notes/includeorrelease-notes/exclude;OS/...) to the associated issuenpm run test -- brave_browser_tests,npm run test -- brave_unit_testswikinpm run presubmitwiki,npm run gn_check,npm run tslintgit rebase master(if needed)Reviewer Checklist:
gnAfter-merge Checklist:
changes has landed on
Test Plan:
Desktop/Android:
--p3a-count-wallet-test-networksflag. Ensure profile contains wallet with testnet tokens.iOS:
Brave Wallet Count Test NetworksBrave Core Switch enabled. Ensure profile contains wallet with testnet tokens.Brave Wallet Count Test Networks. You will have to quit the app and restart for changes to take effect....->Settings->View Chromium Local State...and tapWallet Settings->Networks-> enableShow Test Networksswitch.