Skip to content

pgf/governance disallow non-native transfers#4667

Merged
mergify[bot] merged 7 commits intomainfrom
governance-non-native-transfer-fix
Jun 4, 2025
Merged

pgf/governance disallow non-native transfers#4667
mergify[bot] merged 7 commits intomainfrom
governance-non-native-transfer-fix

Conversation

@Fraccaman
Copy link
Copy Markdown
Collaborator

Describe your changes

Checklist before merging

  • If this PR has some consensus breaking changes, I added the corresponding breaking:: labels
    • This will require 2 reviewers to approve the changes
  • If this PR requires changes to the docs or specs, a corresponding PR is opened in the namada-docs repo
    • Relevant PR if applies:
  • If this PR affects services such as namada-indexer or namada-masp-indexer, a corresponding PR is opened in that repo
    • Relevant PR if applies:

@Fraccaman Fraccaman requested review from grarco and sug0 June 2, 2025 13:25
@Fraccaman Fraccaman marked this pull request as ready for review June 2, 2025 13:25
@github-actions github-actions bot added the breaking:api public API breaking change label Jun 2, 2025
@codecov
Copy link
Copy Markdown

codecov bot commented Jun 2, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 0.00%. Comparing base (29ccb55) to head (0cd4c35).
Report is 26 commits behind head on main.

Additional details and impacted files
@@     Coverage Diff      @@
##   main   #4667   +/-   ##
============================
============================

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Fraccaman
Copy link
Copy Markdown
Collaborator Author

@cwgoes we should always allow credit to pgf and governance addresses right?

@Fraccaman Fraccaman requested review from grarco, sug0 and tzemanovic June 3, 2025 14:27
@Fraccaman Fraccaman added the breaking:consensus Consensus breaking change that requires a hard-fork label Jun 3, 2025
@brentstone
Copy link
Copy Markdown
Collaborator

brentstone commented Jun 3, 2025

@cwgoes we should always allow credit to pgf and governance addresses right?

Anyone should always be allowed to send tokens TO these addresses, yes. @Fraccaman

&& checked!(post_balance - pre_balance)? >= min_funds_parameter
let is_valid_balance = if is_proposal {
if !native_token_address.eq(token) {
return Err(Error::new_alloc(
Copy link
Copy Markdown
Collaborator

@grarco grarco Jun 3, 2025

Choose a reason for hiding this comment

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

I think I've missed this in my first review, this can be new_const too

sug0
sug0 previously requested changes Jun 3, 2025
Copy link
Copy Markdown
Collaborator

@sug0 sug0 left a comment

Choose a reason for hiding this comment

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

I think the code might be fine now, but if possible here are some tests that would be nice to have:

  1. making sure the pgf/gov balance can decrease/increase with governance proposals (with native or non-native tokens)
  2. making sure the pgf/gov balance can increase (with native or non-native tokens)
  3. making sure the pgf/gov balance cannot decrease (with native or non-native tokens)

only a subset of these tests is implemented. it should be relatively easy to impl them, should be a copy/paste of some test and changing a few things

@brentstone
Copy link
Copy Markdown
Collaborator

@sug0 let's leave the remaining tests for another PR. We need to merge this and cut a release ASAP to at least start testing on Housefire

@Fraccaman
Copy link
Copy Markdown
Collaborator Author

Fraccaman commented Jun 4, 2025

making sure the pgf/gov balance can decrease/increase with governance proposals (with native or non-native tokens)

We have an e2e test that does this with IBC

making sure the pgf/gov balance can increase (with native or non-native tokens)

I think i've added test_pgf_non_native_credit and test_governance_non_native_credit to check if balance can increase with non-native tokens

making sure the pgf/gov balance cannot decrease

Same here with test_governance_non_native_debit and test_pgf_non_native_debit

Native token credit is also tested by e2e test and unit test where we check that a proposal was initialized correctly (the proposal author has to deposit some nam). Maybe I can add a test for native token debit.

@Fraccaman Fraccaman requested a review from sug0 June 4, 2025 08:50
@tzemanovic tzemanovic added the merge Ready to merge - mergifyio bot will add the PR to merge queue when all checks pass label Jun 4, 2025
@Fraccaman Fraccaman requested review from sug0 and removed request for sug0 June 4, 2025 12:06
@brentstone brentstone dismissed sug0’s stale review June 4, 2025 12:29

Will impl remaining tests later. Need merged now

mergify bot added a commit that referenced this pull request Jun 4, 2025
@mergify mergify bot merged commit b2c3aff into main Jun 4, 2025
28 checks passed
@mergify mergify bot deleted the governance-non-native-transfer-fix branch June 4, 2025 13:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking:api public API breaking change breaking:consensus Consensus breaking change that requires a hard-fork merge Ready to merge - mergifyio bot will add the PR to merge queue when all checks pass

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants