aptos-core icon indicating copy to clipboard operation
aptos-core copied to clipboard

[API][SDK][CLI] Add backward compatibility testing

Open banool opened this issue 2 years ago • 2 comments

Currently we only test:

  • Server X is compatible with client X.
  • Devnet server is compatible with latest published client release.

We need to test:

  • Server X + 1 is compatible with client X (backwards compatibility).

Which by induction through CI on every PR should prevent any unintentional breaking changes in either direction.

Server in this case is the node API, client is all of our SDKs and the CLI.

Any change that does break compatibility must first go through a very clear deprecation period. There needs to be some way to signal to the CI that the breaking change is okay once we've finally gone through that cycle.

Note, we should not test:

  • Server X is compatible with client X + 1 (forwards compatibility).

If we tested this in addition to the previous one, it'd be impossible to ever actually make a change to the client. We always expect the server to upgrade first, it to still be compatible with the previous client, and then have clients upgrade to the new client. Clients should not be upgraded before servers.

banool avatar Sep 13 '22 17:09 banool

Copy paste of message relevant to this issue that is affecting forge:

The problem here is forge is using a newer client with an older server. We should maintain backwards compatibility, where a newer server should still work with an older client, but not the other way around, otherwise it’d be impossible to ever make a release since server and client are upgraded separately.

So there are two ways to fix this:

  • Rollback the PR (https://github.com/aptos-labs/aptos-core/pull/4012).
  • Make the forge tests only ever use client version <= server version.

Option 2 is the only correct option I feel, otherwise we’re making the assertion that client changes on main must be compatible with the server on an older branch (e.g. testnet), which prevents anyone from ever landing breaking client changes to main (aka halt development).

(How the PR ever passed in the first place is a separate mystery, but you can see that it indeed did pass forge compat testing, which it shouldn't have since the testing at the time is not meant to allow this client +1 change).

banool avatar Sep 13 '22 20:09 banool

Don't take this issue to heart right now, we're still discussing expectations around compatibility / release process.

banool avatar Sep 13 '22 20:09 banool

This issue is stale because it has been open 45 days with no activity. Remove the stale label or comment - otherwise this will be closed in 15 days.

github-actions[bot] avatar Nov 07 '22 03:11 github-actions[bot]