-
Notifications
You must be signed in to change notification settings - Fork 544
make pallet evm dispatches call, create public #358
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
Conversation
sorpaas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Please also change pallet-evm version to 3.0.1-dev and recursively all upper crates before merging!
| sp-io = { version = "3.0.0", default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "frontier" } | ||
| frame-support = { version = "3.0.0", default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "frontier" } | ||
| pallet-evm = { version = "3.0.0", default-features = false, path = "../.." } | ||
| pallet-evm = { version = "3.0.1-dev", default-features = false, path = "../.." } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any bump of dependency should be recursive. See https://github.com/paritytech/frontier#versioning
Alternatively, since it's a patch and no interface is changing, you can also just revert those dependency bumps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I reverted the dependency bumps. There are no interface changes so I think this is appropriate.
frame/evm/Cargo.toml
Outdated
| [package] | ||
| name = "pallet-evm" | ||
| version = "3.0.1-dev" | ||
| version = "3.0.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You at least need to have this!
|
I'm going to try something else to fix my problem and if it doesn't work, I'll come back to this and do the recursive bump that you mentioned earlier. |
|
The thing I tried worked, I don't need this change. |
This comment has been minimized.
This comment has been minimized.
|
@girazoki showed me this https://github.com/PureStake/crowdloan-rewards/blob/main/src/tests.rs#L290 which enables calling private runtime methods |
|
Indeed proxying through pallet_utility::batch is an effective workaround for Moonbeam. This will still be needed for testing any runtime that doesn't use pallet_utility. And it makes the tests more readable even if utility is around. Maybe we can re-open it when time permits. (And we should do |
|
I meant that I'm using the same pattern to make the private call. It declares a Call type generic over the Runtime which is able to form the call correctly. Here is the code but the call still fails and I haven't been able to figure out why moonbeam-foundation/moonbeam@97fc054#diff-4a6734b9be4e30f208c6afca6c81c41fca067292e6e5aef519e8cb0c8be84122R204 So this pattern enables us to call private runtime methods without making them public. This means we don't need to make these methods public for testing as far as I understand. |
This makes testing easier for moonbeam-foundation/moonbeam#339 and I'm unaware of negative consequences of making this change. If there is a reason this shouldn't be done, I want to understand why.