Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sync With Upstream #30

Merged
merged 170 commits into from
Jul 11, 2024
Merged

Sync With Upstream #30

merged 170 commits into from
Jul 11, 2024

Conversation

l-monninger
Copy link
Collaborator

@l-monninger l-monninger commented Jul 2, 2024

Resolves #41

Description

Test Plan

@@ -152,7 +152,7 @@ impl TransactionsApi {
operation_id = "get_transactions",
tag = "ApiTags::Transactions"
)]
async fn get_transactions(
pub async fn get_transactions(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure we need to expose all this stuff from the API crate.

## Unreleased
- Add balance command to easily get account balances for APT currently

## [3.4.1] - 2024/05/31
Copy link
Collaborator

Choose a reason for hiding this comment

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

The aptos-turned-movement crate will need to be re-synced from the latest upstream to benefit from all the accumulated goodness and retain compat.

Or perhaps, expose the CLI entry points and relocate the movement binary to the movement repo, so that the delta here is minimized to feature-gating the unneeded code and deps.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Re-syncing is done in #43.

Copy link

@andygolay andygolay Jul 10, 2024

Choose a reason for hiding this comment

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

Would it work to rename the associated aptos directory to movement ?

@@ -82,7 +82,7 @@ impl CliCommand<Vec<serde_json::Value>> for ListAccount {
account
} else {
return Err(CliError::CommandArgumentError(
"Please provide an account using --account or run aptos init".to_string(),
"Please provide an account using --account or run movement init".to_string(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

On the other hand, there's this branding hardcoded anyway, so perhaps there is no benefit in maintaining an external binary.

crates/aptos/src/genesis/mod.rs Outdated Show resolved Hide resolved
crates/aptos/src/genesis/mod.rs Outdated Show resolved Hide resolved
// Update in-memory state of the database and the metrics before reverting.
// Note that any failures in persisting the revert should be treated as
// non-recoverable.
fn pre_revert(
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is also a cherry-pick from #28, not yet used anywhere, but will be once that PR lands.

Comment on lines +554 to +558
let lock = self.reset_lock();
lock.reset();
}

pub fn reset_lock(&self) -> ResetLock<'_> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Another cherry-pick from #28; it looks like a needless refactoring now, but will come useful later.

testsuite/forge/Cargo.toml Show resolved Hide resolved
testsuite/smoke-test/Cargo.toml Show resolved Hide resolved
testsuite/testcases/Cargo.toml Show resolved Hide resolved
@mzabaluev
Copy link
Collaborator

The changes look sensible now, apart from some minor issues. The only real concern that jumped at me is the added todo! macros, potentially introducing crashiness.

@mzabaluev
Copy link
Collaborator

mzabaluev commented Jul 9, 2024

The merge breaks the movement CLI (in ./crates/aptos), because it updates many of its dependencies, and resurrects others in a broken form, by seemingly resolving "removed by us/changed by them" conflicts in favor of the usually incomplete set of files changed upstream. The conflicts in Cargo.toml are also resolved in favor of resurrecting these dependencies. The code of the CLI crate, however, is retained at the old revision it was pulled from for Monza.

I think the sensible way forward is to completely check out the required crates from main, and solve #41 as part of this so that the movement CLI is up-to-date against its workspace dependencies. Unfortunately, this means partially overwriting some changes made for Monza. We can keep them in the monza branch and revisit if we decide to pick up work on that project again.

@l-monninger
Copy link
Collaborator Author

My mistake, I misused --all-targets I guess: rust-lang/cargo#10958

mzabaluev added 5 commits July 9, 2024 20:05
When syncing with upstream, merge conflicts of the "changed by them/
removed by us" kind have been resolved by updating the conflicting
files to the latest upstream revision. This only partially recovered
the source for crates that have been removed for Monza.
Restore the crates fully.

Some of the changes made for Monza are overwritten from upstream
to reduce the delta and allow resurrecting some crates erased
in the work for Monza:
- ring dependency removal from aptos-crypto, moving the noise
  implementation to aptos-network;
- removal of tokio synchronization slots from aptos-types.

This means this branch no longer supports Monza, and those changes
would need to be re-applied if the Monza direction is taken again.
The storage interface used by tests added by Movement
needed to be updated to the upstream changes.
Pull the README from upstream and remove the duplicate workspace entry.
…m-fixups

Sync With Upstream: fixups, updating the CLI to 3.5.0
The changes do not work if the "multi-threaded-cf" feature
is enabled. We only tried this for Monza.
crate aptos renamed to movement.
We only parameterized FinalityView with Arc<dyn DbReader>,
so just bake it in rather than relying on a generic impl.
When DbReaderWriter is refactored, we might get a better shot at
improving this.
It was made private again in the merge.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Resync movement CLI with latest upstream aptos CLI
4 participants