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: adapt tests to the pre-approval flow #137

Merged
merged 21 commits into from
Oct 8, 2024

Conversation

SebastianElvis
Copy link
Member

@SebastianElvis SebastianElvis commented Oct 7, 2024

Resolves #121
Resolves #119

This PR adapts the tests under x/btcstaking to the pre-approval flow. This includes

  • generalising the h.CreateDelegation function to support both pre-approval and non-pre-approval flow
  • new test util function for submitting inclusion proof
  • adapt most of the tests to use pre-pre-approval

In addition, this PR proposes to rename status VERIFIED to APPROVED for clarity: verified is a bit generic while approved is more consistent with existing terms (pre-"approval"). Also this PR fixes the flaky FuzzBTCDelegation test (checked the fix via fuzzing locally).

@SebastianElvis SebastianElvis marked this pull request as ready for review October 7, 2024 04:13
@SebastianElvis SebastianElvis requested a review from a team as a code owner October 7, 2024 04:13
@SebastianElvis SebastianElvis requested review from KonradStaniec and RafilxTenfen and removed request for a team October 7, 2024 04:13
@KonradStaniec
Copy link
Collaborator

In addition, this PR proposes to rename status VERIFIED to APPROVED for clarity: verified is a bit generic while approved is more consistent with existing terms

ah we wanted to specifically avoid the word approval (https://github.com/babylonlabs-io/pm/pull/48#issuecomment-2382252055) as it has some negative connotations. In reality covenant members do not approve anything. Their job is to:

  1. verify transactions
  2. send signatures
    hence VERIFIED sounded like a good candidate. Do you maybe have other ideas for naming ?

@SebastianElvis
Copy link
Member Author

SebastianElvis commented Oct 7, 2024

In reality covenant members do not approve anything.

Actually I thought the opposite: covenant committee indeed approves BTC delegations, and they can selectively approve and disapprove. Hence the feature we are adding is called pre-approval.

Although I agree that the word does not sound right politically. Let me change it back (UPDATE: fixed)

CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@KonradStaniec KonradStaniec left a comment

Choose a reason for hiding this comment

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

LGTM!

@SebastianElvis SebastianElvis merged commit bbdfa4e into main Oct 8, 2024
18 checks passed
@SebastianElvis SebastianElvis deleted the pre-approval-flow-test branch October 8, 2024 06:48
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.

Flaky test FuzzBTCDelegation Adapt x/btcstaking unit tests to pre-approval flow
3 participants