Skip to content

Conversation

0xIcarus
Copy link
Contributor

@0xIcarus 0xIcarus commented Aug 13, 2025

Summary

  • RFCs: $\emptyset$.
  • Categories: networks, misc.

Deprecating governed gas pool and replacing gas collection logic with transaction_fee::collect_fee module using a fw upgrade. Added e2e tests to confirm deprecation and correct fee collection values and to query for the module on chain.

Changelog

  • Removed all usage of ggp.
  • Updated the framework ref to match the latest ref in branch 0xIcarus/ggp-deprecate on movementlabsxyz/aptos-core to allow feature COLLECT_AND_DISTRIBUTE_GAS_FEES to be voted through.
  • Added e2e tests to make sure gas collection, feature enablement and ggp deprecation works as expected.

Testing

New e2e tests added to verify if fee collection behavior matches expectations
Run using nix develop --command bash -c "just movement-full-node native build.setup.test-e2e-verify-collect-fee --keep-project"

Outstanding issues

@0xIcarus 0xIcarus changed the title 0x icarus/ggp deprecate feat: deprecate ggp and add e2e tests to check for correct gas collection using collect_fee Aug 13, 2025
@0xIcarus 0xIcarus marked this pull request as ready for review August 28, 2025 16:13
process-compose -f process-compose/movement-full-node/process-compose.test-e2e-verify-collect-fee.yml up --wait --follow

test-e2e-framework-upgrade-collect-gas-fees:
process-compose -f process-compose/movement-full-node/process-compose.test-e2e-framework-upgrade-collect-gas-fees.yml up --wait --follow
Copy link
Contributor

Choose a reason for hiding this comment

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

You can remove this. E2E tests require a full-node setup and faucet. We are probably going to run it with

 just movement-full-node native build.setup.test-e2e-verify-collect-fee
 just movement-full-node native build.setup.
test-e2e-framework-upgrade-collect-gas-fees

format!("unknown release string: {}", release).into(),
)
.into()),
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The code formatting is off.

Copy link
Contributor

@musitdev musitdev left a comment

Choose a reason for hiding this comment

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

I don't see where the ggp is deprecated. In the aptos-framework-pre-l1-merge-release crate it's enable.

enable_feature_flags.push(AptosFeatureFlag::GOVERNED_GAS_POOL);

Perhaps it's in the Aptos PR, can you link it in the description?

Comment on lines +107 to +110
faucet_client
.create_account(beneficiary.address())
.await
.context("Failed to create beneficiary account via faucet")?;
Copy link
Contributor

@musitdev musitdev Sep 4, 2025

Choose a reason for hiding this comment

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

On mainnet there's no faucet. You test for the sender, but not for the beneficiary? I think this call fail on mainnet. I think you don't need to create the account to send it some token.
Another remarks, you should send less than 1000 move because on mainnet they will be lost. Beneficiary is a random generated account.

In fact, you don't expect to run the test on mainnet so we don't verify the change work well on mainnet in this case. Perhaps during the migration we should test these change apply correctly. I add some strange issue with the epoch duration change where I got different behavior depending on the network.
So passing the test on a mainnet fork should be a good thing, but in this case you don't care the token amount. I'll try to pass it on a mainnet fork.

genesis.set_sequence_number(acct.inner().sequence_number);
}
let txh = coin_client
.transfer(&mut genesis, sender.address(), 1_000_000, None)
Copy link
Contributor

Choose a reason for hiding this comment

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

Use less move they will be lost in mainnet.

@musitdev
Copy link
Contributor

musitdev commented Sep 5, 2025

I've tested to update mainnet db. The aptos-framework-pre-l1-merge-release-tool works well. I didn't manage to pass the test. The faucet is blocking the end-to-end (E2E) test execution, and I try to verify the on-chain value with an RPC query, but I couldn't find how to obtain the correct value. The RPC calls always fails
From my test the important point I saw is that the aptos-framework-pre-l1-merge-release-tool compiles all scripts, which takes time. For the mainnet migration, we must use precompiled scripts. It will significantly decrease the migration time.
The other point, precompiled move script guarantee that we use the right compiler, which is not guaranteed on the migration instance.

@0xIcarus 0xIcarus force-pushed the 0xIcarus/ggp-deprecate branch from a7b7117 to 14c952a Compare September 10, 2025 10:53
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.

3 participants