Buggity fixes: two small bug fixes #3558
Conversation
On these chains, the code attempted to interact with an undefined spot oracle key, which meant the price lookup failed. Unfortunately due to the way lookups are batched, this meant that adding a network with assets that did not have a spot price oracle defined killed all price lookups.
This ensures that by the time internal signers hit the redux store, the internal signers are known to be unlocked. This is useful because the store ignores emitted signers while the signers are known to be locked, to avoid clearing the signers list at lock time (which emits an empty list of working signers for any services that are relying on that list to know what can currently be actively signed).
| @@ -347,6 +345,8 @@ export default class InternalSignerService extends BaseService<Events> { | |||
| } | |||
|
|
|||
| this.#unlock() | |||
| this.#emitInternalSigners() | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
We haven't implemented that yet but I don't think it's useful to remove it.
| @@ -152,6 +152,10 @@ const getRatesForTokens = async ( | |||
| } | |||
| }[] | |||
| > => { | |||
| if (SPOT_PRICE_ORACLE_CONSTANTS[network.chainID] === undefined) { | |||
There was a problem hiding this comment.
This looks safe but I couldn't really trigger this line by testing UI, what are the testing steps here?
There was a problem hiding this comment.
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.
|
Because the scope of the internal signers change is pretty tight here, I'm going to bypass review by @mhluongo in the interest of shipping. |
Two fixes here, one for a small edge case in internal signer emission
that meant that if for some reason the internal signers data was cleared
in redux, it would never be correctly repopulated (expectation is that
there's no production scenario that would really trigger this).
The other is an issue where adding a network to the extension that didn't
have an associated spot price oracle listed would cause price lookups for
that network to fail, and this would cascade into failing all price lookups.
The correct fix for the latter is probably to make price lookups less sensitive
to individual failure, but for now I'm making the spot price oracle bit be
safe for networks that don't have an entry in the spot price oracle list.
Latest build: extension-builds-3558 (as of Thu, 20 Jul 2023 10:37:18 GMT).