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

210 add finalization pallet #214

Merged
merged 7 commits into from
Nov 18, 2022
Merged

210 add finalization pallet #214

merged 7 commits into from
Nov 18, 2022

Conversation

letodunc
Copy link
Contributor

Implemented pallet finalizer methods

@letodunc letodunc added the enhancement New feature or request label Nov 17, 2022
@letodunc letodunc added this to the M4 milestone Nov 17, 2022
@letodunc letodunc self-assigned this Nov 17, 2022
@letodunc letodunc linked an issue Nov 17, 2022 that may be closed by this pull request
@codecov-commenter
Copy link

Codecov Report

Merging #214 (be233d2) into main (06758d2) will increase coverage by 1.58%.
The diff coverage is 96.02%.

@@            Coverage Diff             @@
##             main     #214      +/-   ##
==========================================
+ Coverage   79.61%   81.19%   +1.58%     
==========================================
  Files          53       57       +4     
  Lines        4571     5047     +476     
==========================================
+ Hits         3639     4098     +459     
- Misses        932      949      +17     
Impacted Files Coverage Δ
assets/pallet_template/src/lib.rs 0.00% <ø> (ø)
assets/pallet_template/src/weights.rs 0.00% <0.00%> (ø)
pallets/finalizer/src/weights.rs 0.00% <0.00%> (ø)
pallets/onboarding/src/types.rs 100.00% <ø> (ø)
runtime/src/lib.rs 12.22% <ø> (ø)
pallets/finalizer/src/mock.rs 72.72% <72.72%> (ø)
pallets/finalizer/src/lib.rs 88.23% <88.23%> (ø)
pallets/finalizer/src/tests.rs 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Member

@ilhanu ilhanu left a comment

Choose a reason for hiding this comment

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

Nice work, this pretty much concludes the work of the finalizer-pallet.

@@ -21,6 +21,7 @@ pub enum AssetStatus {
PURCHASED,
REJECTED,
SLASH,
CANCELLED,
Copy link
Member

Choose a reason for hiding this comment

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

Great to have gotten Cancelled, it may be a bit undefined yet what we do with this status.
What happens to the deposit of the seller ? Since the seller was the one initiating and cancelling there should be a fee to pay. This isn't comment on the PR, just a feedback potential issue for the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With Cancelled status, we know that the transaction didn't succeed not because of the notary but because of the seller. For the moment, cancelling is free, but later, a slash will be applied to the seller.

}

#[test]
fn validate_transaction_asset_should_succeed() {
Copy link
Member

Choose a reason for hiding this comment

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

@letodunc you think we should extend this test till share-distributor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we can relay on the tests in the bidding pallet for the process_finalised_assets(). This is the step following the finalizer pallet execution.

@letodunc letodunc merged commit 933a3b9 into main Nov 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
No open projects
Status: No status
Development

Successfully merging this pull request may close these issues.

Add Finalization pallet
3 participants