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

CSUB-1232: Remove collect coins #1660

Merged
merged 27 commits into from
Aug 9, 2024
Merged

CSUB-1232: Remove collect coins #1660

merged 27 commits into from
Aug 9, 2024

Conversation

beqaabu
Copy link
Contributor

@beqaabu beqaabu commented Jul 30, 2024

Description of proposed changes

Removing collect_coins functionality. Everything is commented out for now not deleted. The only part left to figure out is the migrations. The node compiles, the tests are green

Practical tips for PR review & merge:

  • All GitHub Actions report PASS
  • Newly added code/functions have unit tests
    • Coverage tools report all newly added lines as covered
    • The positive scenario is exercised
    • Negative scenarios are exercised, e.g. assert on all possible errors
    • Assert on events triggered if applicable
    • Assert on changes made to storage if applicable
  • Modified behavior/functions - try to make sure above test items are covered
  • Integration tests are added if applicable/needed

Copy link

For full LLVM coverage report click here!

@beqaabu beqaabu changed the title Remove collect coins CSUB-1232: Remove collect coins Jul 30, 2024
Copy link
Contributor

@atodorov atodorov left a comment

Choose a reason for hiding this comment

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

Added some comments and questions

pallets/creditcoin/src/benchmarking.rs Outdated Show resolved Hide resolved
pallets/creditcoin/src/lib.rs Outdated Show resolved Hide resolved
pallets/creditcoin/src/ocw/rpc.rs Show resolved Hide resolved
pallets/creditcoin/src/ocw/tasks.rs Outdated Show resolved Hide resolved
pallets/creditcoin/src/ocw/tasks.rs Outdated Show resolved Hide resolved
pallets/creditcoin/src/types.rs Outdated Show resolved Hide resolved
pallets/creditcoin/src/types/collect_coins.rs Outdated Show resolved Hide resolved
pallets/creditcoin/src/weights.rs Outdated Show resolved Hide resolved
runtime/src/tests.rs Outdated Show resolved Hide resolved
runtime/src/tests.rs Outdated Show resolved Hide resolved
@beqaabu beqaabu force-pushed the remove-collect-coins branch 2 times, most recently from 46a82c3 to f6751d0 Compare August 6, 2024 09:41
Copy link
Contributor

@atodorov atodorov left a comment

Choose a reason for hiding this comment

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

Minor comments, otherwise looks good.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
pallets/creditcoin/src/migrations/v6.rs Outdated Show resolved Hide resolved
@beqaabu beqaabu requested a review from atodorov August 7, 2024 10:19
Copy link
Contributor

@atodorov atodorov left a comment

Choose a reason for hiding this comment

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

For some reason compiling unit tests is failing:

error[E0275]: overflow evaluating the requirement `<Runtime as pallet_grandpa::Config>::KeyOwnerProof == _`

https://github.com/gluwa/creditcoin/actions/runs/10282472003/job/28455986492?pr=1660

pallets/creditcoin/src/weights.rs Show resolved Hide resolved
@beqaabu
Copy link
Contributor Author

beqaabu commented Aug 8, 2024

For some reason compiling unit tests is failing:

error[E0275]: overflow evaluating the requirement `<Runtime as pallet_grandpa::Config>::KeyOwnerProof == _`

https://github.com/gluwa/creditcoin/actions/runs/10282472003/job/28455986492?pr=1660

saw this gluwa/creditcoin3@1655f7d and this paritytech/polkadot-sdk#2641 and then I remembered when we fixed this issue on cc3, probably needs the same fix here, @atodorov WDYT?

@beqaabu beqaabu marked this pull request as ready for review August 8, 2024 08:01
@atodorov
Copy link
Contributor

atodorov commented Aug 8, 2024

saw this gluwa/creditcoin3@1655f7d and this paritytech/polkadot-sdk#2641 and then I remembered when we fixed this issue on cc3, probably needs the same fix here, @atodorov WDYT?

I think it is safer and faster to revert the change to rust-toolchain.toml instead.

atodorov
atodorov previously approved these changes Aug 8, 2024
Copy link
Contributor

@atodorov atodorov left a comment

Choose a reason for hiding this comment

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

  • cargo audit - unrelated failure, known issue
  • build-creditcoin-transaction-producer - unrelated failure, known issue
  • llvm cov - fails with not enough disk space on a GitHub hosted runner. This is new but not convinced it is entirely related to the current changes. cargo test appears to be passing -> https://pastebin.com/iJkEbE50

LGTM

DylanVerstraete
DylanVerstraete previously approved these changes Aug 8, 2024
@beqaabu beqaabu requested a review from dlebee August 8, 2024 11:23
pallets/creditcoin/src/lib.rs Outdated Show resolved Hide resolved
pallets/creditcoin/src/benchmarking.rs Outdated Show resolved Hide resolved
pallets/creditcoin/src/lib.rs Outdated Show resolved Hide resolved
pallets/creditcoin/src/migrations/v8.rs Show resolved Hide resolved
pallets/creditcoin/src/tests.rs Outdated Show resolved Hide resolved
@beqaabu beqaabu dismissed stale reviews from DylanVerstraete and atodorov via 5bdd35b August 8, 2024 16:12
dlebee
dlebee previously approved these changes Aug 8, 2024
atodorov
atodorov previously approved these changes Aug 8, 2024
@beqaabu beqaabu merged commit 1fd01a6 into dev Aug 9, 2024
35 of 38 checks passed
@beqaabu beqaabu deleted the remove-collect-coins branch August 9, 2024 08:51
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