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

Test Mint and Burn #1010

Open
wants to merge 29 commits into
base: main
Choose a base branch
from
Open

Test Mint and Burn #1010

wants to merge 29 commits into from

Conversation

Primata
Copy link
Contributor

@Primata Primata commented Jan 20, 2025

Summary

  • RFCs: Link to RFC, Link to RFC, or $\emptyset$.
  • Categories: any of protocol-units, networks, scripts, util, cicd, or misc.

#1009

Burn elsa's supply.

Changelog

moved away from native_bridge integration to e2e testing

added burn_from and mint_to methods gated to framework signer

Testing

networks/movement/movement-client/src/bin/e2e/lock_mint.rs

rm -rf .movement/ && RUST_LOG=info CELESTIA_LOG_LEVEL=FATAL CARGO_PROFILE=release CARGO_PROFILE_FLAGS=--release nix develop --extra-experimental-features nix-command --extra-experimental-features flakes --command bash -c "just movement-full-node native build.setup.eth-local.celestia-local.test-mint-burn --keep-tui"

Outstanding issues

We might need to add a burn_from entry function on the core_resources_account.

@Primata Primata requested a review from andygolay as a code owner January 21, 2025 00:19
@0xmovses
Copy link
Contributor

We all try to stick to this commit message standard (roughly)
https://www.conventionalcommits.org/en/v1.0.0/

Copy link
Contributor

@andygolay andygolay left a comment

Choose a reason for hiding this comment

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

When I run

RUST_LOG=info CELESTIA_LOG_LEVEL=FATAL CARGO_PROFILE=release CARGO_PROFILE_FLAGS=--release nix develop --extra-experimental-features nix-command --extra-experimental-features flakes --command bash -c "just movement-full-node native build.setup.eth-local.celestia-local.test-lock-mint --keep-tui"

I get

Error: Failed to create dead account

caused by

API error Error(VmError): Invalid transaction: Type: Validation Code: SEQUENCE_NUMBER_TOO_OLD

image

@Primata
Copy link
Contributor Author

Primata commented Jan 25, 2025

When I run


RUST_LOG=info CELESTIA_LOG_LEVEL=FATAL CARGO_PROFILE=release CARGO_PROFILE_FLAGS=--release nix develop --extra-experimental-features nix-command --extra-experimental-features flakes --command bash -c "just movement-full-node native build.setup.eth-local.celestia-local.test-lock-mint --keep-tui"

I get

Error: Failed to create dead account

caused by

API error Error(VmError): Invalid transaction: Type: Validation Code: SEQUENCE_NUMBER_TOO_OLD

image

@andygolay please clear .movement/ before every run.

Updated command to do it before hand

@0xmovses
Copy link
Contributor

As far I can see there is no NativeBridge.sol deployment on anvil here, and MOVE deposit call. @andygolay not ready for review yet.

@0xmovses 0xmovses marked this pull request as draft January 25, 2025 15:52
@Primata Primata requested a review from andygolay January 26, 2025 03:17
@0xmovses 0xmovses changed the title Test Lock and Mint Test Mint and Burn Jan 26, 2025
@Primata Primata marked this pull request as ready for review January 27, 2025 02:57
Copy link
Contributor

@0xmovses 0xmovses left a comment

Choose a reason for hiding this comment

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

Thanks for this. Remove all println!s for tracing::info. Cleanup imports, use tree style imports. The comments you've written right at the bottom are correct but lets remove the comment from the SC. Rename the test to mint_burn.rs and any associated process_compose naming and cmd / pkgs.

Thanks!

@0xmovses
Copy link
Contributor

Merge conflicts will auto-resolve after:

  1. Governed Gas Pool aptos-core#114 (review) is approved and merged
  2. PR updates movement/main aptos deps to use branch = movement

@Primata Primata requested a review from 0xmovses January 27, 2025 04:19
Copy link
Contributor

@andygolay andygolay left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@andygolay andygolay left a comment

Choose a reason for hiding this comment

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

Added one change suggestion, to assert dead address balance is 0 at the end.

networks/movement/movement-client/src/bin/e2e/mint_burn.rs Outdated Show resolved Hide resolved
@0xmovses
Copy link
Contributor

Even when I remove .movement/ here I get the error

Error: Failed to create dead account

Caused by:
    API error Error(VmError): Invalid transaction: Type: Validation Code: SEQUENCE_NUMBER_TOO_OLD

Wrong increment?

@0xmovses
Copy link
Contributor

0xmovses commented Feb 4, 2025

Is there a new dependent PR in aptos-core that contains the mint_to function? O

@Primata
Copy link
Contributor Author

Primata commented Feb 4, 2025

Is there a new dependent PR in aptos-core that contains the mint_to function? O

Same branch and PR add_burn_from

@0xmovses
Copy link
Contributor

0xmovses commented Feb 4, 2025

PR number or link?

@0xmovses
Copy link
Contributor

0xmovses commented Feb 6, 2025

Ok this is ready to go in can you fix the merge conflicts and add this test to the Checks.yml all new e2e test should be added to CI.

@l-monninger
Copy link
Collaborator

Will pull again.

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.

4 participants