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

Update bridge framework tests #80

Open
wants to merge 2 commits into
base: movement
Choose a base branch
from

Conversation

andygolay
Copy link

Description

In aptos-move/e2e-move-tests/src/tests/bridge.rs:

  • Update test_counterparty so anyone with the pre-image can call atomic_bridge_counterparty::complete_bridge_transfer
  • Update test_initiator so initiator calls atomic_bridge_initiator::complete_bridge_transfer
  • Add test_refund and test_abort

In aptos-move/framework/aptos-framework/sources/atomic_bridge.move:

Type of Change

  • [ x] New feature
  • [ x] Bug fix
  • Breaking change
  • Performance improvement
  • Refactoring
  • Dependency update
  • Documentation update
  • Tests

Which Components or Systems Does This Change Impact?

  • Validator Node
  • Full Node (API, Indexer, etc.)
  • Move/Aptos Virtual Machine
  • [ x] Aptos Framework
  • Aptos CLI/SDK
  • Developer Infrastructure
  • Other (specify)

How Has This Been Tested?

  • Move unit tests: movement move test
  • Rust E2E tests: cargo test bridge

Key Areas to Review

  • If there is some security reason why only the bridge operator should be allowed to call atomic_bridge_initiator::complete_bridge_transfer, then we should update our audited modules. However, I don't see why it would be an issue, if the counterparty lock and abort calls are restricted to the operator.

Checklist

  • [ x] I have read and followed the CONTRIBUTING doc
  • [ x] I have performed a self-review of my own code
  • [ x] I have commented my code, particularly in hard-to-understand areas
  • [ x] I identified and added all stakeholders and component owners affected by this change as reviewers
  • [ x] I tested both happy and unhappy path of the functionality
  • [ x] I have made corresponding changes to the documentation

Copy link
Collaborator

@mzabaluev mzabaluev 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, though I don't feel knowledgeable enough to approve.

Should we enroll these tests into the CI?
The story of GitHub workflows in this fork is that we removed anything from upstream that wasn't consciously used by us. But any workflow from the main branch can be picked up and adapted to run on movement, including those that ran the move tests.

@andygolay
Copy link
Author

Looks good, though I don't feel knowledgeable enough to approve.

Should we enroll these tests into the CI? The story of GitHub workflows in this fork is that we removed anything from upstream that wasn't consciously used by us. But any workflow from the main branch can be picked up and adapted to run on movement, including those that ran the move tests.

Yes good point, I believe @andyjsbell is looking into how to fix up the CI.

@andygolay andygolay marked this pull request as draft October 15, 2024 13:34
@andygolay andygolay marked this pull request as ready for review October 15, 2024 13:40
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.

2 participants