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

Add dummy precompile hook with rust and typescript tests #478

Merged
merged 2 commits into from
Mar 27, 2024

Conversation

ParthDesai
Copy link
Contributor

@ParthDesai ParthDesai commented Mar 27, 2024

Description

Follow up PR for Moonsong-Labs/moonkit#32

This PR utilizes the hooks introduced in above PR and adds revert bytecode at the evm account derived from the asset id specified in the hook invocation. It also has rust unit test and typescript integration test.

@ParthDesai ParthDesai requested a review from girazoki March 27, 2024 04:42
@ParthDesai ParthDesai added B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes D3-trivial PR contains trivial changes in a runtime directory that do not require an audit D5-nicetohaveaudit⚠️ PR contains trivial changes to logic that should be properly reviewed. breaking Needs to be mentioned in breaking changes labels Mar 27, 2024
Copy link
Contributor

github-actions bot commented Mar 27, 2024

Coverage Report

(master)

@@                   Coverage Diff                   @@
##           master   add-dummy-precompile     +/-   ##
=======================================================
  Coverage   64.77%                 64.77%   0.00%     
  Files          66                     66             
  Lines        9775                   9775             
=======================================================
  Hits         6331                   6331             
  Misses       3444                   3444             
Files Changed Coverage

Coverage generated Wed Mar 27 08:42:46 UTC 2024

Copy link
Collaborator

@girazoki girazoki left a comment

Choose a reason for hiding this comment

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

Small comments, but otherwise looks good!

@@ -439,3 +469,17 @@ pub type ForeignFungiblesTransactor = FungiblesAdapter<
/// Multiplier used for dedicated `TakeFirstAssetTrader` with `ForeignAssets` instance.
pub type AssetRateAsMultiplier =
AssetFeeAsExistentialDepositMultiplier<Runtime, WeightToFee, AssetRate, ForeignAssetsInstance>;

#[test]
fn test_asset_id_to_account_conversion() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we don't need this test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently asset_id_to_account takes prefix as slice, instead of hard bound array. Then it does invoke copy_from_slice, which can panic, if the slice's size is not exact.

If the implementation changes tomorrow (i.e the expected size of the slice in the implementation), it need to be captured by the unit test (otherwise you would see panic during integration test or worse in dev environment). Which this test prevents.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's discuss this offline. I am merging this as it is not blocker as of now.

@ParthDesai ParthDesai merged commit 97bfa27 into master Mar 27, 2024
32 checks passed
@ParthDesai ParthDesai deleted the add-dummy-precompile branch March 27, 2024 12:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes breaking Needs to be mentioned in breaking changes D3-trivial PR contains trivial changes in a runtime directory that do not require an audit D5-nicetohaveaudit⚠️ PR contains trivial changes to logic that should be properly reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants