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.

Sign up for GitHub

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

Jump to bottom

Buggity fixes: two small bug fixes #3558

Merged
merged 3 commits into from
Jul 20, 2023
Merged

Buggity fixes: two small bug fixes #3558

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
449b053
Fix errors with price oracles on unsupported chains
Shadowfiend Jul 20, 2023
7543954
Emit internal signers after unlocking in internal signers
Shadowfiend Jul 20, 2023
f0a49fc
Merge branch 'main' into buggity-fixes
jagodarybacka Jul 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions background/lib/priceOracle.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,10 @@ const getRatesForTokens = async (
}
}[]
> => {
if (SPOT_PRICE_ORACLE_CONSTANTS[network.chainID] === undefined) {
Copy link
Contributor

@jagodarybacka jagodarybacka Jul 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This looks safe but I couldn't really trigger this line by testing UI, what are the testing steps here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops, did it in a hurry and forgot to add test instructions 🤦 Adding a custom asset on a custom chain and then waiting for the price alarm to trigger price checks I think will do the trick here. Background script should log an error without this, no error with it.

return []
}

const multicall = new ethers.Contract(
MULTICALL_CONTRACT_ADDRESS,
MULTICALL_ABI,
Expand Down
4 changes: 2 additions & 2 deletions background/services/internal-signer/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,8 +334,6 @@ export default class InternalSignerService extends BaseService<Events> {
this.#hiddenAccounts = {
...plainTextVault.hiddenAccounts,
}

this.#emitInternalSigners()
}
}

Expand All @@ -347,6 +345,8 @@ export default class InternalSignerService extends BaseService<Events> {
}

this.#unlock()
this.#emitInternalSigners()
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't it making ignoreExistingVaults argument useless? As we are always emitting signers after this change. RIght now we are only using it during password creation anyway which means there are no keys stored yet anyway. To keep this code clear I would remove ignoreExistingVaults argument.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That argument would result in cleared vaults here, which would clear the signers in redux--which is what we would want in a scenario where we were resetting a password, which is the non-onboarding reason that argument is in place, see the function docs:

extension/background/services/internal-signer/index.ts

Lines 262 to 263 in 7543954

* This option makes sense if a user has lost their password, and needs
* to generate a new vault.

We haven't implemented that yet but I don't think it's useful to remove it.


return true
}

Expand Down