[pallet-revive] Update delegate_call to accept address and weight

[ermalkaleci]

Enhance the delegate_call function to accept an address target parameter instead of a code_hash. This allows direct identification of the target contract using the provided address. Additionally, introduce parameters for specifying a customizable ref_time limit and proof_size limit, thereby improving flexibility and control during contract interactions.

[ermalkaleci]

@xermicus @athei Can someone review this please?

[ermalkaleci]

This implementation will use the wrong immutable data. Reason is that the contract info of the caller is used in the Frame. You probably need to add the callees address in the DelegatedCall structure and use that in the immutable_data() function.

Do you mean my implementation is wrong or currently access to immutable is incorrect?

[athei]

The bug existed before. Introduced with immutable variable support. But passing the address makes it possible to resolve this bug. Hence it should be fixed here.

[ermalkaleci]

The bug existed before. Introduced with immutable variable support. But passing the address makes it possible to resolve this bug. Hence it should be fixed here.

Did a quick check at it seems the immutable is read for account of top frame which will be the callee. https://github.com/paritytech/polkadot-sdk/blob/01936b346098e4c603f035622dfd24846ac5cb52/substrate/frame/revive/src/exec.rs#L1600 Also there's a test for it https://github.com/paritytech/polkadot-sdk/blob/01936b346098e4c603f035622dfd24846ac5cb52/substrate/frame/revive/src/exec.rs#L4597 Am I missing something?

[athei]

seems the immutable is read for account of top frame which will be the callee

This is wrong. delegate_call has the callers contract info on top of the frame. That is the whole point of delegate call: Executing a different code in the context of the caller.

Passed here: https://github.com/ermalkaleci/polkadot-sdk/blob/a5f2f1c8444859b1f69a0e4a050ac5a116f31cb4/substrate/frame/revive/src/exec.rs#L1423

You shadow the callees contract info with the callers one herre: https://github.com/ermalkaleci/polkadot-sdk/blob/a5f2f1c8444859b1f69a0e4a050ac5a116f31cb4/substrate/frame/revive/src/exec.rs#L1417

[ermalkaleci]

@athei Because of the way immutable is implemented, I thought the logic is correct but it's not. Immutable ends on EVM bytecode but we keep it as storage. Delegate call should not access immutable of caller but callee. All clear now

[athei]

Exactly. The current logic is wrong.

[athei]

Okay I found the place where you modified immutable_data. It looks fine. But I am confused that the correct_immutable_data_in_delegate_call test didn't need any modification. Can you look into this? Because we didn't seem to properly test that the immutable data of the callee's are used. Or am I getting something wrong?

[ermalkaleci]

correct_immutable_data_in_delegate_call test didn't need any modification

I did modify expected value. Both cases should return 2

[athei]

You are right. I missed that. Indeed it looks correct. Would be nice if you could address the two nits that are still open.

[ermalkaleci]

@athei I did some refactoring, let me know what you think.

[athei]

Can you please not do this refactor. Adding additional redundant data makes the code harder to understand.

[ermalkaleci]

I can easily revert it, but I don't see it as redundant; instead, I find it straightforward and easy to understand.

[ermalkaleci]

Would be nice if you could address the two nits that are still open.

Can you help me understand which one you're referring?

[athei]

I can easily revert it, but I don't see it as redundant; instead, I find it straightforward and easy to understand.

How is having a field account_id and address on the same structure not confusing.

[athei]

Would be nice if you could address the two nits that are still open.

Can you help me understand which one you're referring?

Just the two open remarks. Scroll up.

[ermalkaleci]

So the locking system is essentially useless. Need to think how to proceed. The immutables really make our lifes hard.

Indeed. Maybe we can address it on another PR

[ermalkaleci]

So the locking system is essentially useless. Need to think how to proceed. The immutables really make our lifes hard.

Indeed. Maybe we can address it on another PR

@athei Actually I don't think we have an issue here. If the contract is terminated then you can't delegate calls to that address.

[athei]

So the locking system is essentially useless. Need to think how to proceed. The immutables really make our lifes hard.

Indeed. Maybe we can address it on another PR

@athei Actually I don't think we have an issue here. If the contract is terminated then you can't delegate calls to that address.

But this is exactly the issue. The locking system exists to make sure that you can always delegate. Otherwise somebody could delete the contract and the delegate will fail. The locking system allows you to lock the code hash for a deposit to make sure it can't be deleted.

On Ethereum this is not a problem. They changed the SELFDESTRUCT to not actually remove the contract.

Our options are basically: 1) Change the locking system to lock contracts and not code hashes. This will work but it is still not 100% compatible: On EVM all contracts are locked by default and we would require them to call a custom API. It is also a lot of work to implement probably. 2) Just remove the ability to remove contracts and code. The most compatible solution but provides no incentive to remove code blobs. 3) Remove the locking system and call it a day. Probably the worst of both worlds.

But we should not block your PR for that. Let's get this merged and think about this later. Looks good. Just this one confusion regarding the test.

[ermalkaleci]

I believe removing the locking mechanism is the best course of action here. It's important to be mindful of where and how we delegate function calls. This aligns with the standard EVM design, although SELFDESTRUCT is changed recently.

If we decide to make a contract immortal, we should also reconsider the need for a deposit limit, as it would no longer serve a purpose in such a scenario.