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.

Sign up for GitHub

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

Jump to bottom

feat: thea pallet and improvements to asset-handler pallet #595

Merged
merged 49 commits into from
Jan 18, 2023
Merged

feat: thea pallet and improvements to asset-handler pallet #595

merged 49 commits into from
Jan 18, 2023

Conversation

felixfaisal
Copy link
Contributor

@felixfaisal felixfaisal commented Jan 5, 2023 •
edited
Loading

Describe your changes

Issue ticket number and link

Checklist before requesting a review

  • I have performed a self-review of my code.
  • If it is a core feature, I have added thorough tests.
  • I removed all Clippy and Formatting Warnings.
  • I added required Copyrights.

@felixfaisal
Copy link
Contributor Author

Hey @serhii-temchenko The latest CI build has an error related to dependencies

error[E0658]: use of unstable library feature 'scoped_threads'

Can you take a look into this?

@serhii-temchenko
Copy link
Contributor

Hey @serhii-temchenko The latest CI build has an error related to dependencies

error[E0658]: use of unstable library feature 'scoped_threads'

Can you take a look into this?

Sure, error appeared in the process of formatter installation. Required version (1.5) was outdated and for now in latest rustup installer present 1.5.1 version. So we don't need install formatter separately. I raised the PR #598 with fix of this and some refactoring of workflows declaration. Please review that PR and after merge it - you could pick fix from the Develop branch.

@felixfaisal felixfaisal marked this pull request as ready for review January 9, 2023 07:34
@felixfaisal felixfaisal added the A3-InProgress Pull request is in progress. No review needed at this stage. label Jan 9, 2023
@codecov
Copy link

codecov bot commented Jan 9, 2023 •
edited
Loading

Codecov Report

Base: 54.44% // Head: 53.26% // Decreases project coverage by -1.18% ⚠️

Coverage data is based on head (7e1f277) compared to base (8ec10c8).
Patch coverage: 43.62% of modified lines in pull request are covered.

Additional details and impacted files 
@@             Coverage Diff             @@
##           Develop     #595      +/-   ##
===========================================
- Coverage    54.44%   53.26%   -1.18%     
===========================================
  Files           23       25       +2     
  Lines         1677     1881     +204     
===========================================
+ Hits           913     1002      +89     
- Misses         764      879     +115
Impacted Files Coverage Δ
client/src/lib.rs 0.00% <ø> (ø)
runtime/src/lib.rs 0.00% <0.00%> (ø)
pallets/thea/src/lib.rs 25.17% <25.17%> (ø)
thea-primitives/src/lib.rs 83.33% <83.33%> (ø)
pallets/asset-handler/src/lib.rs 76.09% <93.33%> (+2.95%) ⬆️

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

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@felixfaisal felixfaisal added A0-PleaseReview Pull request needs code review. and removed A3-InProgress Pull request is in progress. No review needed at this stage. labels Jan 9, 2023
Gauthamastro and others added 7 commits January 11, 2023 17:48
@Gauthamastro
Fix imports
f21e7a2
@nakul1010
test cases added
db127f6
@nakul1010
Fixed clippy warnings and added TC
d851dab
@felixfaisal
fix: clippy warnings
81d189c
@felixfaisal
fix: update tests
eb9b0fa
@felixfaisal
refactor: apply rustfmt
930f617
@felixfaisal
feat: Thea Pallet (#597)
b0cc30a 
This PR adds a pallet for Thea and important extrinsics 
- Runtime Inclusion of Pallet 
- Host functions for BLS Functions 
- Mock runtime environment, and tests for bls verification 
- `approve_deposit` extrinsic
@felixfaisal
Asset handler tests improvement (#602)
1a21c5c 
In this PR:
- Common implementation in `thea` related tests moved to separate
private functions to be reused.
- Extended amount of tests to cover more cases
- Added functions with todo's inside for missing cases
@felixfaisal felixfaisal changed the title feat: new extrinsic create_thea_asset for asset_handler feat: thea pallet and improvements to asset-handler pallet Jan 12, 2023
serhii-temchenko
serhii-temchenko requested changes Jan 12, 2023
View reviewed changes
Copy link
Contributor

@serhii-temchenko serhii-temchenko left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Only 3 assertions present in a tests for approve_deposit extrinsic. Please add all required tests and assertions for the thea pallet.

node/src/service.rs Outdated Show resolved Hide resolved
node/src/service.rs Outdated Show resolved Hide resolved
pallets/asset-handler/src/lib.rs Outdated Show resolved Hide resolved
pallets/thea/Cargo.toml Outdated Show resolved Hide resolved
pallets/thea/src/lib.rs Outdated Show resolved Hide resolved
pallets/thea/src/lib.rs Outdated Show resolved Hide resolved
pallets/thea/src/lib.rs Show resolved Hide resolved
pallets/thea/src/lib.rs Outdated Show resolved Hide resolved
pallets/thea/src/lib.rs Outdated Show resolved Hide resolved
pallets/thea/src/lib.rs Outdated Show resolved Hide resolved
@serhii-temchenko
Copy link
Contributor

serhii-temchenko commented Jan 17, 2023 •
edited
Loading

@felixfaisal still 2 comments untouched. Please check them.
Plus nothing about this comment.

@felixfaisal
Copy link
Contributor Author

Hey @serhii-temchenko Tests will be covered under a separate issue - #614

@Gauthamastro Gauthamastro merged commit 16baf4f into Develop Jan 18, 2023
@Gauthamastro Gauthamastro deleted the fx-asset-handler branch January 18, 2023 10:03
@felixfaisal felixfaisal restored the fx-asset-handler branch January 18, 2023 10:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Gauthamastro Gauthamastro Gauthamastro approved these changes

@serhii-temchenko serhii-temchenko serhii-temchenko approved these changes

@zktony zktony Awaiting requested review from zktony

@35359595 35359595 Awaiting requested review from 35359595

Assignees
No one assigned
Labels
A0-PleaseReview Pull request needs code review.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@felixfaisal @serhii-temchenko @Gauthamastro @nakul1010