feat(revive): add contract instantiated event#8789
Conversation
| assert_eq!( | ||
| &events(), | ||
| &[Event::Instantiated { | ||
| deployer: ALICE_ADDR, | ||
| contract: instantiated_contract_address | ||
| }] | ||
| ); |
There was a problem hiding this comment.
This is the only test that was effected.
I've checked there's also a test that already checks the case when the instantiation call succeeds but the contract is reverted - In that case there's already an assertion that checks that no events should be emitted. See instantiation_fails_with_failing_output.
All these tests were done previously, but updated the event assertions when #7164 removed the events. The only one that needed to be updated is this one.
| let result = stack | ||
| .run(executable, input_data) | ||
| .map(|_| (address, stack.first_frame.last_frame_output)) | ||
| .map(|_| (address, stack.first_frame.last_frame_output)); | ||
| if let Ok((contract, ref output)) = result { | ||
| if !output.did_revert() { | ||
| Contracts::<T>::deposit_event(Event::Instantiated { deployer, contract }); | ||
| } | ||
| } | ||
| result |
There was a problem hiding this comment.
I added this within exec, in run_instantiate which is called only from the top frame. The other possibility was to have this in bare_instantiate of the pallet's lib.rs, but I thought it would be better to have it in here since this is the function performing the instantiation (and some tests are actually calling this function directly).
|
CC @pgherveou @athei Can you add the appropriate labels please? 🙏 |
athei
left a comment
athei
left a comment
There was a problem hiding this comment.
Blocked until you convinced me that we need that. Let's discuss in the issue.
|
I am confused. I thought we had performance issues and thus needed to reduce among others this exact event (which is entirely unnecessary anyways)? |
This event (emitted just for the top frame) does NOT have a performance impact. It's actually very obvious that it doesn't have a performance impact, what makes you think that it does? On the other hand, not having this event is going to make the DX quite horrendous and as @voliva has explained several times: removing this event unnecessarily breaks an assumption of previous libraries and tooling. |
|
He even writes in the PR description My position stays the same regardless of how big or small you think the negative impact is. This is entirely unnecessary. I see a problem solved here on-chain which can solved off-chain and this is never good. We are trying to move closer to Ethereum so why add something like this? People start relying on it and then we won't be able to change or remove in the future. |
|
For more context regarding this, please read the conversation in the original issue #8677. This morning I added one comment summarising why we need this and why it wouldn't bring back the performance issue you're mentioning. And yes, I said "This might need refreshing the weights" because I'm not sure myself if emitting one single event in a transaction is worth recalculating the weights, given how insignificant it is towards the total weight. If it is, then sure, let's refresh them. I'll need some help though, as I'm not sure how to run benchmarks in the specified targets while being an external contributor. |
Yes, that's what I'm telling you. The performance impact is completely negligible, obviously. Do you want me to explain to you why?
Then why in the world would you bring the performance impact into the discussion? 😅
It's actually the other way around. By removing this event you are forcing your consumers to rely into an arbitrary implementation detail... because the way that you are calculating this address is not something that has been properly documented or standarized. That formula is just an arbitrary (and probably not that great) implementation detail, which I bet is going to change any day, and when that day comes, then every off-chain consumer will have to deal with the mess. The event, on the other hand, is a clean API that allows us not to have to rely on these arbitrary implementation details. Not to mention the fact that it would also allow you to write better tests, that don't rely on internals. |
|
Ok sorry to let this drag, it's been a couple of busy weeks.
I think we can agree that emitting a single event for the top-level frame has no significant performance impact.
I think our stance is that we should try as much as possible to embrace Ethereum convention and not add new one, as it makes things harder to document, maintain, and can have perfomance implications
These are not arbitrary implementation details, the address derivation logic is defined in the Ethereum yellow paper and in EIPs. Changing it would break the entire ecosystem. That being said I understand that having a single event to rely on would be more convenient for you |
great!
I highly doubt it. However, no one is suggesting this.
"potato, potahto"... I mean, I still think that the current name is totally adequate. However, I'm more than willing to compromise on the name. Just propose a better one and let's go with that. However, let's please not drag this any further b/c of the name.
You are adding a new convention, already! Because with the current implementation the nonce is not only being altered for every new transaction, it's also being altered for every new contract instantiation... So, yeah, no, sorry, this is already different than what Ethereum does, isn't it? If I'm not mistaken, then this test is yielding a false positive. So, yeah, no, sorry, I don't think that's embracing an Ethereum convention. This is trying to be similar, but not the same.
The way that the nonce is being altered (which ends up affecting the address being computed) is what's an implementation detail, and since you are going to change that any day by now, we should not rely on it.
It's not that it's more convenient for me. If this event had been present, this pallet could have had better tests, which would have caught the potential regression that was reported (and ignored) weeks ago. |
what is the false positive here ?
Runtime upgrade are on hold for now so that patch has probably not been deployed, this should get unstuck on Paseo, once this get enacted paseo-network/passet-hub#16 |
I mean, my point is that the test is assuming that the nonce is not being increased prior to the first instantiation, which -looking at the code- doesn't seem like an obvious assumption. What I mean is that the test is not testing the whole flow, and that if there was a bug by which the nonce being used for computing the address is, in fact, the updated one, then that test would yield a false positive. We are trying to actually test this against the latest commit using zombienet, but we haven't succeeded to set it up :/ |
not too sure what you mean, this get the nonce, get the address derived from the nonce, and verify that it matches the address returned by the dry_run, the goal of the test is to test that part.
if you are just testing this, I would simply use kitchensink, or the dev-node that we added recently (under revive/dev-node) |
What I mean is that IMO this assertion should be done in a similar way to what it's being done in here. Meaning that there should be a real transaction performing a real contract instantiation, and the test should check that the address being produced does, in fact, match with the one that comes from the dry-run. The main reason being that by the time that the contract instantiation happens, then the nonce has already being increased. However, there is specific logic that tries to compensate for that. So, if one day this kind of logic changed, then this test would produce a false positive. In short: the output of the dry-run should be tested against the "output" of a real transaction. I think that the instantiated event would be useful for such test.
Thanks a lot for this! we did use it and I'm happy to stand corrected: that the dry_run is currently returning the correct contract address. Thank you! |
…-evt # Conflicts: # substrate/frame/revive/src/exec.rs
|
/cmd fmt |
|
One of the CI checks has failed with a "context cancelled". Is there somethingI need to fix in this PR? |
|
Just random CI failure. Just re-running. |
|
This pull request has been mentioned on Polkadot Forum. There might be relevant details there: |
# Description This PR adds the `Instantiated` event for pallet-revive for the top frame. Addresses issue #8677 This might need refreshing the weights of bot `instantiate` and `instantiate_with_code`, as it emits a new event. ## Integration No additional work is needed to integrate this feature. The pallet will emit on `Instantiated` event every time `instantiate` or `instantiate_with_code` successfully performs an instantiation. # Checklist * [x] My PR includes a detailed description as outlined in the "Description" and its two subsections above. * [x] My PR follows the [labeling requirements]( https://github.com/paritytech/polkadot-sdk/blob/master/docs/contributor/CONTRIBUTING.md#Process ) of this project (at minimum one label for `T` required) * External contributors: ask maintainers to put the right label on your PR. * [x] I have made corresponding changes to the documentation (if applicable) * [x] I have added tests that prove my fix is effective or that my feature works (if applicable) --------- Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Alexander Theißen <alex.theissen@me.com>
github-project-automation
Bot
moved this from Todo
to Done
in [preview] release tracker
6 participants
Description
This PR adds the
Instantiatedevent for pallet-revive for the top frame. Addresses issue #8677This might need refreshing the weights of bot
instantiateandinstantiate_with_code, as it emits a new event.Integration
No additional work is needed to integrate this feature. The pallet will emit on
Instantiatedevent every timeinstantiateorinstantiate_with_codesuccessfully performs an instantiation.Checklist
Trequired)