Skip to content

Conversation

@MajorLift
Copy link
Contributor

@MajorLift MajorLift commented Nov 15, 2023

Explanation

  • Releases major version of network-controller, which enables usage of updated getEthQuery messenger action.
  • Releases major version of transaction-controller, which includes a large number of significant changes.
  • Releases major version of assets-controllers, which includes breaking changes in NfTController.
  • Releases patch versions of {ens,gas-fee,polling,queued-request,selected-network}-controller, which are downstream of network-controller.

References

Changelog

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've highlighted breaking changes using the "BREAKING" category above as appropriate

@MajorLift MajorLift added the team-wallet-framework Deprecated: Please use `team-core-platform` instead. label Nov 15, 2023
@MajorLift MajorLift self-assigned this Nov 15, 2023
@MajorLift MajorLift requested a review from a team as a code owner November 15, 2023 18:34
Copy link
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

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

According to the scenario that we outlined here, since we are now bumping network-controller by a major and since all of the other packages depend on network-controller, I'm curious whether this scenario is possible:

  • Today, a client is using the getEthQuery messenger action to get the ethQuery that network-controller exports. The type of this instance is defined in eth-query 3.x, so they arguments they pass to ethQuery.sendAsync match this version as well.
  • Now, they upgrade network-controller to the new major version without making any other changes. Now the type of ethQuery comes from eth-query 4.x, so those arguments no longer match the type, and they get a type error.

Given this, should we actually go ahead and release these other packages too?

@MajorLift MajorLift force-pushed the release/91.0.0 branch 2 times, most recently from f3c100c to 14a0d49 Compare November 15, 2023 21:32
@Gudahtt
Copy link
Member

Gudahtt commented Nov 15, 2023

There were a few more small breaking changes I was considering for the controller messenger; perhaps we could hold off on this release for a day or so to evaluate whether we can include those too? I don't want to hold up the release train any longer than we need to, but base controller breaking changes can be quite disruptive, so minimizing them might be worthwhile.

Edit: Oh I'm sorry, nevermind. I've just noticed that the base controller isn't included in this.

@MajorLift
Copy link
Contributor Author

MajorLift commented Nov 15, 2023

@Gudahtt Keeping this release on hold shouldn't be an issue, but maybe the base-controller updates could be batched into its own release for ease of review, especially since it will include patch releases for all of the controllers?

Edit: No worries!

@Gudahtt
Copy link
Member

Gudahtt commented Nov 15, 2023

Having a release just for the base controller changes is a great idea. I was thinking the same thing

@MajorLift
Copy link
Contributor Author

@mcmire I added releases for all of the downstream packages!

Gudahtt
Gudahtt previously approved these changes Nov 15, 2023
Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM! But I'd definitely appreciate someone from the confirmation systems team taking a second look at the transaction controller changelog, I didn't review that as closely as the rest

Gudahtt
Gudahtt previously approved these changes Nov 15, 2023
Co-authored-by: Matthew Walsh <matthew.walsh@consensys.net>
@MajorLift
Copy link
Contributor Author

@matthewwalsh0 Suggestions applied here: b9c6623. Thank you for going through this in detail! The transaction-controller changelog reads a lot smoother now.

Copy link
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

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

Looks good!

@MajorLift MajorLift merged commit 65c3c4c into main Nov 16, 2023
@MajorLift MajorLift deleted the release/91.0.0 branch November 16, 2023 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

team-wallet-framework Deprecated: Please use `team-core-platform` instead.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants