Repository navigation
Add chainId to every transaction - #349
Conversation
Gudahtt
left a comment
Gudahtt
left a comment
There was a problem hiding this comment.
I'm not sure what to think of this "backwards compatibility" strategy 🤔 I think it will ensure things are no worse than they are today, but it'll still result in some interactions with the wrong transaction (e.g. if you have two networks with the same network ID but different chain IDs).
What's worse, this flaw will apply not just to old transactions, but to new ones as well.
I can approve this anyway (after tests have been added to pass the coverage requirements on CI), since it's less broken than before. But this remains somewhat broken. Maybe this could be improved in the future by removing the "networkID" property for new transactions, and only considering that for older transactions.
| @@ -255,14 +256,15 @@ export class TransactionController extends BaseController<TransactionConfig, Tra | |||
| * @param currentNetworkID - string representing the current network id | |||
There was a problem hiding this comment.
Nit: The additional parameter should be documented here:
| * @param currentNetworkID - string representing the current network id | |
| * @param currentNetworkID - string representing the current network id | |
| * @param currentChainId - The current chain ID |
There was a problem hiding this comment.
I'm not sure what to think of this "backwards compatibility" strategy 🤔 I think it will ensure things are no worse than they are today, but it'll still result in some interactions with the wrong transaction (e.g. if you have two networks with the same network ID but different chain IDs).
What's worse, this flaw will apply not just to old transactions, but to new ones as well.
I can approve this anyway (after tests have been added to pass the coverage requirements on CI), since it's less broken than before. But this remains somewhat broken. Maybe this could be improved in the future by removing the "networkID" property for new transactions, and only considering that for older transactions.
@Gudahtt I am 100% with you on all your comments. The end goal is to remove networkID completely. We are doing this incrementally because we still use it in some parts of the app so I thought it would be safer to keep it but always prefer chainId over it.
I have changed some part of the code to be sure that chainId is always preferred, we only fallback to networkID in case chainId is not there. For all new transactions, chainId will always be added, so at least we know that for new transactions, networkID will not be used.
We had a change some time ago where we basically did
const currentNetworkID = network.state.provider.chainId;
which could cause problems like you mentioned: could result in the wrong set of transactions being returned whenever the chainId was different from the networkId.
I now differentiate them:
const currentChainId= network.state.provider.chainId;
const currentNetworkID= network.state.network;
In this way, chainId and networkID at least can now have different values resolving that problem for now, but yes the objective is still to remove networkID completely in the near future.
Let me know what are your thoughts on this and if it's ok to merge!
There was a problem hiding this comment.
That sounds good to me! Better to make these changes incrementally if possible
Gudahtt
left a comment
Gudahtt
left a comment
There was a problem hiding this comment.
LGTM! I can re-review after the conflict is resolved.
# Conflicts: # src/transaction/TransactionController.ts
This is the release candidate for version 12.2.1. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > Bumps version to 12.2.1 and updates CHANGELOG entries and comparison links for the release. > > - **Release 12.2.1**: > - Bump `package.json` version to `12.2.1`. > - Update `CHANGELOG.md`: > - Add `## [12.2.1]` section. > - Update reference links: `[Unreleased]` compares from `v12.2.1...HEAD`; add `[12.2.1]` compare link. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 1e8174fec420a2338f0f39494746d9424e89cd10. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY --> --------- Co-authored-by: github-actions <github-actions@github.com> Co-authored-by: Jiexi Luan <jiexiluan@gmail.com>
3 participants
Uh oh!
There was an error while loading. Please reload this page.