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

fix: prevent recursion error with pallet_collective metadata #412

Merged
merged 2 commits into from
Feb 17, 2025

Conversation

AlexD10S
Copy link
Collaborator

@AlexD10S AlexD10S commented Feb 14, 2025

Context
pop-cli does not support extrinsics that take another call as a parameter (e.g Utility.batch). Instead, when parsing the chain metadata, it returns a FunctionNotSupported error for such extrinsics and notifies the user that the function is not supported:

 Select the function to call:
│  ○ as_derivative 
│  ● batch (Function Not Supported)

Bug
A bug was discovered when testing the Pop Network mainnet runtime (r0gue-io/pop-node#448), where metadata retrieval for the Council pallet (pallet_collective) caused a recursion error.

The problem is because pallet_collective defines type Proposal = RuntimeCall; (see) rather than referencing RuntimeCall directly, as other pallets do (see).

To Replicate:

pop up parachain -f ./networks/mainnet.toml
pop call chain --url ws://localhost:9944/ 

It also can be tested in a live network (Mythos), that has a council governance.

pop call chain --url wss://mythos.ibp.network

Fix
A new check has been included when parsing metadata to properly identify RuntimeCall at any level within the namespace path. Instead of assuming a direct reference, the system now inspects all segments of the path to detect and handle RuntimeCall correctly (link from scale-info library).

Note
I've created a unit test for this issue using a live chain with the same implementation as the Pop Network mainnet. A TODO comment is included to update the test once the Pop Network goes live.

[sc-2948]

@AlexD10S AlexD10S marked this pull request as ready for review February 14, 2025 15:43
@AlexD10S AlexD10S requested a review from al3mart February 14, 2025 15:43
Copy link

codecov bot commented Feb 14, 2025

Codecov Report

Attention: Patch coverage is 87.50000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 75.18%. Comparing base (00680f8) to head (0774706).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
crates/pop-parachains/src/call/metadata/params.rs 87.50% 0 Missing and 2 partials ⚠️
@@            Coverage Diff             @@
##             main     #412      +/-   ##
==========================================
+ Coverage   75.16%   75.18%   +0.02%     
==========================================
  Files          63       63              
  Lines       13870    13886      +16     
  Branches    13870    13886      +16     
==========================================
+ Hits        10425    10440      +15     
+ Misses       2118     2117       -1     
- Partials     1327     1329       +2     
Files with missing lines Coverage Δ
crates/pop-parachains/src/call/metadata/params.rs 91.66% <87.50%> (-0.38%) ⬇️

... and 1 file with indirect coverage changes

Copy link
Contributor

@al3mart al3mart left a comment

Choose a reason for hiding this comment

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

Nice! Super quick action 👍

It feels that we are being forced to work a bit ad-hoc around these cases we are hitting.

@AlexD10S AlexD10S merged commit a2b9184 into main Feb 17, 2025
32 checks passed
@AlexD10S AlexD10S deleted the bug/fix-parse-metadata branch February 17, 2025 07:40
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.

2 participants