Allow simple ETH transfer proposals

[Siegrift]

See: https://api3workspace.slack.com/archives/C02ALA11APM/p1683621470320789

Rationale

Review commit by commit.

Initially, we used only a very loose validation of the proposal form. Later we improved this, but we also prevented a use case of simple ETH transfer.

To support this use case, I've added special cases to the validation. If both target contract signature and parameters are empty, the validation passes and a proposal can be created. Since signature and parameters can be empty now, I think it makes sense to hide them in proposal details.

I've tested it manually locally, but creating e2e is complex so I opted for encoding unit tests only.

Images

1. I filled the proposal form

   

2. After creating it, it got immediately executed (since I have all the voting power)

   

3. Proposal details

   

[bbenligiray]

I'd expect that parameters would need to be [] instead of null

[Siegrift]

I'd expect that parameters would need to be [] instead of null

The thing is that [] has already a well defined meaning - empty parameters for a contract call. I personally like this distinction.

[Siegrift]

I've mainly got a question around whether it makes sense to allow a zero value for these simple ETH transfer proposals.

Good point. We probably should not.

[bbenligiray]

ethers.utils.defaultAbiCoder.encode([], []) is 0x and ethers.utils.defaultAbiCoder.encode([], null) throws. null is not a valid value for something to be ABI-encoded. Maybe have the whole parameters textbox disappear when the signature is empty.

I've mainly got a question around whether it makes sense to allow a zero value for these simple ETH transfer proposals.

There is an edge case where someone may want to send a value with some parameters encoded but no function specified so that the target's fallback function gets invoked. However, the current implementation doesn't allow that (because it wouldn't know how to encode the parameter values with an empty signature) so it would be a good idea to not allow 0 value when the signature is empty.

[Siegrift]

ethers.utils.defaultAbiCoder.encode([], []) is 0x and ethers.utils.defaultAbiCoder.encode([], null) throws. null is not a valid value for something to be ABI-encoded.

I mean this is handled in the implementation. We never pass null during encoding.

Maybe have the whole parameters textbox disappear when the signature is empty.

I think both target contract signature and parameters should be together so I dislike the suggestion. To most clear UI/UX to me is hide it behind accordeon called "contract call parameters" which the user could expand if necessary. Not sure if it's worth implementing.

There is an edge case where someone may want to send a value with some parameters encoded but no function specified so that the target's fallback function gets invoked.

Good point. Btw. this will not work with our metadata format even if we would like to support it and we would need a new version for this.

[mcoetzee]

To most clear UI/UX to me is hide it behind accordeon called "contract call parameters" which the user could expand if necessary. Not sure if it's worth implementing.

Hmm seeing as these simple ETH transfer proposals isn't the norm, I would rather keep things as is.

[Siegrift]

Hmm seeing as these simple ETH transfer proposals isn't the norm, I would rather keep things as is.

I wouldn't say "it's not a norm" - it feels pretty natural to me. But I agree with keeping things as is as the current UX/UI seems good enough for me.

I have enforced the validation to only allow the proposals if both signature and parameters are empty. So even if you would like to use [] the validation error will tell you. @bbenligiray let me know if this is fine - If you prefer [] strongly I can do it that way.

[bbenligiray]

ethers.utils.defaultAbiCoder.encode([], []) is 0x and ethers.utils.defaultAbiCoder.encode([], null) throws. null is not a valid value for something to be ABI-encoded.

I meant this in the sense that there is a convention for using [] when there is nothing to ABI-encode. Similarly, if you compile contract EmptyContract {} the contract ABI will be []. Obviously these are not exactly the same, but it's enough for me to try putting in [] instead of leaving it empty on my first try (as you can see on my screenshot on Slack).

[Siegrift]

I meant this in the sense that there is a convention for using [] when there is nothing to ABI-encode. Similarly, if you compile contract EmptyContract {} the contract ABI will be []. Obviously these are not exactly the same, but it's enough for me to try putting in [] instead of leaving it empty on my first try (as you can see on my screenshot on Slack).

I understand your reasoning, but that comes form your deep understanding of how things work under the hood. When you create an ETH transfer proposal (and there is no contract function to be called) both parameters and address don't apply. I think with that perspective it's more reasonable to let them empty (not touch/fill them at all). But as I said, I can change that if you want it your way.

With this new implementation the validation would let you know that you want them to be empty. Does this sound OK ?

[bbenligiray]

With this new implementation the validation would let you know that you want them to be empty.

It doesn't need to. It should error when parameters is empty and it should encode them as usual when it's [] (and get 0x considering that types is also []).

[Siegrift]

It doesn't need to. It should error when parameters is empty and it should encode them as usual when it's [] (and get 0x considering that types is also []).

Ok. I've changed the implementation to accept [] instead of empty string (see the image below).

[bbenligiray]

Thanks

[Siegrift]

I should have squash merged - sorry.