Use chainId for incoming transactions controller
#9583
Merged
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
Builds ready [979e881]
Page Load Metrics (403 ± 47 ms)
|
Gudahtt
force-pushed
the
use-chain-id-in-incoming-transactions-controller
branch
from
979e881 to
05f4a0a
Compare
Builds ready [05f4a0a]
Page Load Metrics (404 ± 51 ms)
|
rekmarks
reviewed
Oct 14, 2020
Gudahtt
force-pushed
the
use-chain-id-in-incoming-transactions-controller
branch
from
05f4a0a to
e974580
Compare
rekmarks
requested changes
Oct 14, 2020
Gudahtt
force-pushed
the
use-chain-id-in-incoming-transactions-controller
branch
from
e974580 to
afa60a0
Compare
Builds ready [afa60a0]
Page Load Metrics (397 ± 59 ms)
|
|
I'm going to move most of these unit test changes to a separate PR, and get that merged ahead of this. These changes deserve separate explanation. I'll put this in draft until then. |
Gudahtt
force-pushed
the
use-chain-id-in-incoming-transactions-controller
branch
from
afa60a0 to
83aa309
Compare
Builds ready [83aa309]
Page Load Metrics (482 ± 62 ms)
|
Gudahtt
added a commit
that referenced
this pull request
Oct 29, 2020
The shared mocks used previously in the incoming transaction controller tests have been replaced with functions that can generate a new mock for each test. We should avoid ever sharing mocks between tests. It's quite easy for a mock to get accidentally mutated or not correctly "reset" for the next test, leading to test inter-dependencies and misleading results. In particular, it is unsafe to share a `sinon` fake (e.g. a spy or stub) because they can't be fully reset between tests. Or at least it's difficult to reset them property, and it can't be done while also following their recommendations for preventing memory leaks. The spy API and all related state can be reset with `resetHistory`, which can be called between each test. However `sinon` also recommends calling `restore` after each test, and this will cause `sinon` to drop its internal reference to the fake object, meaning any subsequent call to `resetHistory` would fail. This is intentional, as it's required to prevent memory from building up during the test run, but it also means that sharing `sinon` fakes is particularly difficult to do safely. Instead we should never share mocks in the first place, which has other benefits anyway. This was discovered while writing tests for #9583. I mistakenly believed that `sinon.restore()` would reset the spy state, and this was responsible for many hours of debugging test failures.
Gudahtt
added a commit
that referenced
this pull request
Oct 29, 2020
Unit tests have been added to the incoming transactions controller to ensure that block updates are correctly resulting in state updates when incoming transactions are enabled. All other events that trigger state updates are tested as well. The tests were written to be minimally dependent upon implementation details of the controller itself. `nock` was used to mock the API response from Etherscan. Each event is triggered asynchronously by `sinon`, as in production they are likely only triggered asynchronously. This was extracted from #9583 This PR includes a new `waitUntilCalled` module meant to help with writing asynchronous tests. It allows you to wait until a stub has been called.
Gudahtt
added a commit
that referenced
this pull request
Oct 29, 2020
The shared mocks used previously in the incoming transaction controller tests have been replaced with functions that can generate a new mock for each test. We should avoid ever sharing mocks between tests. It's quite easy for a mock to get accidentally mutated or not correctly "reset" for the next test, leading to test inter-dependencies and misleading results. In particular, it is unsafe to share a `sinon` fake (e.g. a spy or stub) because they can't be fully reset between tests. Or at least it's difficult to reset them property, and it can't be done while also following their recommendations for preventing memory leaks. The spy API and all related state can be reset with `resetHistory`, which can be called between each test. However `sinon` also recommends calling `restore` after each test, and this will cause `sinon` to drop its internal reference to the fake object, meaning any subsequent call to `resetHistory` would fail. This is intentional, as it's required to prevent memory from building up during the test run, but it also means that sharing `sinon` fakes is particularly difficult to do safely. Instead we should never share mocks in the first place, which has other benefits anyway. This was discovered while writing tests for #9583. I mistakenly believed that `sinon.restore()` would reset the spy state, and this was responsible for many hours of debugging test failures.
Gudahtt
added a commit
that referenced
this pull request
Oct 29, 2020
Unit tests have been added to the incoming transactions controller to ensure that block updates are correctly resulting in state updates when incoming transactions are enabled. All other events that trigger state updates are tested as well. The tests were written to be minimally dependent upon implementation details of the controller itself. `nock` was used to mock the API response from Etherscan. Each event is triggered asynchronously by `sinon`, as in production they are likely only triggered asynchronously. This was extracted from #9583 This PR includes a new `waitUntilCalled` module meant to help with writing asynchronous tests. It allows you to wait until a stub has been called.
Gudahtt
added a commit
that referenced
this pull request
Oct 30, 2020
Unit tests have been added to the incoming transactions controller to ensure that block updates are correctly resulting in state updates when incoming transactions are enabled. All other events that trigger state updates are tested as well. The tests were written to be minimally dependent upon implementation details of the controller itself. `nock` was used to mock the API response from Etherscan. Each event is triggered asynchronously by `sinon`, as in production they are likely only triggered asynchronously. This was extracted from #9583 This PR includes a new `wait-until-called` module meant to help with writing asynchronous tests. It allows you to wait until a stub has been called.
Gudahtt
force-pushed
the
use-chain-id-in-incoming-transactions-controller
branch
from
83aa309 to
a9cf635
Compare
Builds ready [a9cf635]
Page Load Metrics (473 ± 65 ms)
|
The incoming transactions controller now uses the `chainId` for the current network instead of the `networkId`. This ensures that custom RPC endpoints for the built-in supported networks do correctly receive incoming transactions. As part of this change, the incoming transactions controller will also cease keeping track of the "last block fetched" for networks that are not supported. This piece of state never really represented the last block fetched, as _no_ blocks were fetched for any such networks. It been removed.
Gudahtt
force-pushed
the
use-chain-id-in-incoming-transactions-controller
branch
from
a9cf635 to
89cbc8a
Compare
|
@rekmarks This is finally now ready for review again! |
Builds ready [89cbc8a]
Page Load Metrics (380 ± 47 ms)
|
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
The incoming transactions controller now uses the
chainIdfor the current network instead of thenetworkId. This ensures that custom RPC endpoints for the built-in supported networks do correctly receive incoming transactions.As part of this change, the incoming transactions controller will also cease keeping track of the "last block fetched" for networks that are not supported. This piece of state never really represented the last block fetched, as no blocks were fetched for any such networks. It been removed.