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 Delegation through Proxy Pallet #2533

Merged
merged 9 commits into from
Oct 31, 2023
Merged

Fix Delegation through Proxy Pallet #2533

merged 9 commits into from
Oct 31, 2023

Conversation

ahmadkaouk
Copy link
Contributor

What does it do?

What important points reviewers should know?

Is there something left for follow-up PRs?

What alternative implementations were considered?

Are there relevant PRs or issues in other repositories (Substrate, Polkadot, Frontier, Cumulus)?

What value does it bring to the blockchain users?

@ahmadkaouk ahmadkaouk added B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes D9-needsaudit👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited labels Oct 19, 2023
@github-actions
Copy link
Contributor

Coverage generated "Thu Oct 19 10:05:42 UTC 2023":
https://d3ifz9vhxc2wtb.cloudfront.net/pulls/2533/html/index.html

Master coverage: 79.91%
Pull coverage:

@ahmadkaouk ahmadkaouk changed the title Proxy Contract Fix Delegation through Proxy Pallet Oct 19, 2023
@ahmadkaouk ahmadkaouk marked this pull request as ready for review October 19, 2023 17:50
@ahmadkaouk ahmadkaouk added the not-breaking Does not need to be mentioned in breaking changes label Oct 19, 2023
@librelois
Copy link
Collaborator

@ahmadkaouk you need to account for 1 more db read in the weights for proxy.proxy in file runtime/common/src/weights/pallet_proxy.rs

@ahmadkaouk
Copy link
Contributor Author

@ahmadkaouk you need to account for 1 more db read in the weights for proxy.proxy in file runtime/common/src/weights/pallet_proxy.rs

@librelois Is it necessary? I believe the filter check is performed in validate_transaction, which is called by the Client before the transaction enters the transaction pool. Additionally, in this case, the error is returned by rpc-core "RPC-CORE: submitExtrinsic(extrinsic: Extrinsic): Hash:: 1010: Invalid Transaction: Transaction call is not expected." Do we still need to consider a database read in this scenario?

@librelois
Copy link
Collaborator

librelois commented Oct 30, 2023

@ahmadkaouk you need to account for 1 more db read in the weights for proxy.proxy in file runtime/common/src/weights/pallet_proxy.rs

@librelois Is it necessary? I believe the filter check is performed in validate_transaction, which is called by the Client before the transaction enters the transaction pool. Additionally, in this case, the error is returned by rpc-core "RPC-CORE: submitExtrinsic(extrinsic: Extrinsic): Hash:: 1010: Invalid Transaction: Transaction call is not expected." Do we still need to consider a database read in this scenario?

Yes it's necessary, because the filter is re-executed in the block execution (all transaction validation checks are redone during block execution, to ensure that the block contains only valid transactions).

@librelois
Copy link
Collaborator

@ahmadkaouk can you add a reminder message in the script scripts/benchmarking.sh, to make sure that the engineer that will run this script will not erase the manual change?

@ahmadkaouk ahmadkaouk merged commit 456027e into master Oct 31, 2023
27 of 28 checks passed
@ahmadkaouk ahmadkaouk deleted the ahmad-proxy-contract branch October 31, 2023 13: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 D9-needsaudit👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited not-breaking Does not need to be mentioned in breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants