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

Removal of the Living Assets Ownership Pallet #171

Merged
merged 2 commits into from
Nov 14, 2023
Merged

Conversation

ccubu
Copy link
Contributor

@ccubu ccubu commented Nov 10, 2023

PR Type:

Refactoring


PR Description:

This PR focuses on the removal of the Living Assets Ownership Pallet from the ownership-chain project. The changes include:

  • Deletion of the Living Assets Ownership Pallet related files.
  • Removal of the related precompiles from the precompiles module.
  • Adjustment of the Runtime configuration in the precompiles module.

PR Main Files Walkthrough:

files:
  • ownership-chain/runtime/src/precompiles/mod.rs: The Living Assets Ownership Pallet related precompiles and types have been removed. This includes the removal of the LivingAssetsPrecompile and Erc721 types, and their associated execution in the execute function. The is_precompile function has also been updated to remove the check for collection addresses.
  • ownership-chain/pallets/living-assets-ownership/src/lib.rs: The entire file has been deleted as part of the removal of the Living Assets Ownership Pallet.
  • ownership-chain/precompile/living-assets/src/lib.rs: The entire file has been deleted as it was part of the Living Assets Ownership Pallet precompiles.
  • ownership-chain/precompile/erc721/src/lib.rs: The entire file has been deleted as it was part of the ERC721 precompiles associated with the Living Assets Ownership Pallet.
  • ownership-chain/pallets/living-assets-ownership/Cargo.toml: The entire file has been deleted as it was the Cargo.toml file for the Living Assets Ownership Pallet.

@ccubu
Copy link
Contributor Author

ccubu commented Nov 10, 2023

/describe

@github-actions github-actions bot changed the title remove ownship pallet Removal of the Living Assets Ownership Pallet Nov 10, 2023
@ccubu
Copy link
Contributor Author

ccubu commented Nov 10, 2023

/review

Copy link

PR Analysis

  • 🎯 Main theme: Removal of the Living Assets Ownership Pallet
  • 📝 PR summary: This PR focuses on the removal of the Living Assets Ownership Pallet from the ownership-chain project. The changes include deletion of the Living Assets Ownership Pallet related files, removal of the related precompiles from the precompiles module, and adjustment of the Runtime configuration in the precompiles module.
  • 📌 Type of PR: Refactoring
  • 🧪 Relevant tests added: No
  • ⏱️ Estimated effort to review [1-5]: 3, because the PR involves removal of a significant part of the codebase and requires a good understanding of the project structure and dependencies to ensure nothing crucial is being removed unintentionally.
  • 🔒 Security concerns: No security concerns found

PR Feedback

  • 💡 General suggestions: The PR seems to be well-structured and the removal of the Living Assets Ownership Pallet appears to be done thoroughly. However, it would be beneficial to ensure that all dependencies related to the removed pallet are also removed to avoid any potential issues in the future. Additionally, it would be helpful to provide a reason for the removal of the pallet in the PR description for better context.

  • 🤖 Code feedback:

    • relevant file: ownership-chain/runtime/src/lib.rs
      suggestion: Ensure that the removal of the Living Assets Ownership Pallet does not affect the functionality of the remaining code. [important]
      relevant line: -pub use pallet_living_assets_ownership;

    • relevant file: ownership-chain/runtime/src/precompiles/mod.rs
      suggestion: Confirm that the removal of the Living Assets Ownership Pallet related precompiles and types does not impact the execution of the remaining precompiles. [important]
      relevant line: -use pallet_evm_erc721::Erc721Precompile;

    • relevant file: ownership-chain/precompile/utils/macro/src/lib.rs
      suggestion: Ensure that the changes in the generate_function_selector function do not introduce any unexpected behavior. [medium]
      relevant line: return Err(revert("input is too short"));

    • relevant file: ownership-chain/precompile/utils/src/data.rs
      suggestion: Confirm that the changes in the EvmDataReader struct methods do not introduce any unexpected behavior. [medium]
      relevant line: return Err(revert("pointer points out of bounds"));

How to use

To invoke the PR-Agent, add a comment using one of the following commands:
/review [-i]: Request a review of your Pull Request. For an incremental review, which only considers changes since the last review, include the '-i' option.
/describe: Modify the PR title and description based on the contents of the PR.
/improve [--extended]: Suggest improvements to the code in the PR. Extended mode employs several calls, and provides a more thorough feedback.
/ask <QUESTION>: Pose a question about the PR.
/update_changelog: Update the changelog based on the PR's contents.

To edit any configuration parameter from configuration.toml, add --config_path=new_value
For example: /review --pr_reviewer.extra_instructions="focus on the file: ..."
To list the possible configuration parameters, use the /config command.

@ccubu ccubu linked an issue Nov 13, 2023 that may be closed by this pull request
@ccubu ccubu marked this pull request as ready for review November 13, 2023 13:53
@asiniscalchi
Copy link
Member

the change request expose a problem in the design. However we won't address it now but fix the behavior.

@@ -61,18 +49,12 @@ where
// Non-Frontier specific nor Ethereum precompiles :
// a if a == hash(1024) => Some(Sha3FIPS256::execute(handle)),
a if a == hash(1025) => Some(ECRecoverPublicKey::execute(handle)),
a if a == hash(1026) => Some(LivingAssetsPrecompile::execute(handle)),
Copy link

Choose a reason for hiding this comment

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

should be removed from pub fn used_addresses() as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

fix format

format

removed test

add runtime to mocks

fix precompile test

Fix conflicts
@ccubu ccubu merged commit 3a09f2e into main Nov 14, 2023
7 checks passed
@ccubu ccubu deleted the feat/remove-own-pallet branch November 14, 2023 09:33
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.

Remove Ownership Pallet from Caladan
2 participants