feat(protocol): Add votes and snapshot plugins to BridgedERC20 #15715

adaki2004 · 2024-02-09T13:15:19Z

So we were discussing "delegation" during the airdrop. I'm fine with it but if i recall, Daniel and Keng vouched for a one transaction: claim and delegation bundled together (so from inside the airdrop claiming contract).

To achieve this we can use the delegateBySig() function.

Daniel's advice was make BridgeERC20 implement ERC20Votes and ERC20Snapshot since this way all bridge contracts could have this feature and we dont need to make extra changes to the protoocl to work with Taiko tokens. (I could not put the modules into BridgedERC20Base because of inheritance linearization)

packages/protocol/contracts/tokenvault/BridgedERC20.sol

dantaik · 2024-02-09T13:58:48Z

Dani, I also suggest suggest this into two PRs, one to add snapshot and delegate function to the bridgedERC20 token; the other one to solve the airdrop-and-delegate issue -- if we can do it in on tx, it's great, otherwise, we can still use more than one transaction, but BridgedERC20 should not use tx.origin just because we have the airdrop-and-delegate objective.

dantaik · 2024-02-09T14:11:03Z

We can use this function in lib/openzeppelin-contracts-upgradeable/contracts/token/ERC20/extensions/ERC20VotesUpgradeable.sol:

  /**
     * @dev Delegates votes from signer to `delegatee`
     */
    function delegateBySig(
        address delegatee,
        uint256 nonce,
        uint256 expiry,
        uint8 v,
        bytes32 r,
        bytes32 s
    ) public virtual override {
  ...

Before the airdrop transaction, we collect a delegateSignature from the user, making sure its valid; then the user proceed with the airdrop claim function. Two signatures, one transaction.

Later, we asynchronously fire many delegateBySig transactions behind the scene. The misson will be accomlished this way.

We can also ask the user to transact his delegateBySig transaction after receiving the airdrop.

adaki2004 · 2024-02-09T15:39:50Z

Dani, I also suggest suggest this into two PRs, one to add snapshot and delegate function to the bridgedERC20 token; the other one to solve the airdrop-and-delegate issue -- if we can do it in on tx, it's great, otherwise, we can still use more than one transaction, but BridgedERC20 should not use tx.origin just because we have the airdrop-and-delegate objective.

I rather do it in one PR here - as this is the reason why we doing it. The changes are not so big, i dont think we shall fractionalize that much.

adaki2004 · 2024-02-09T17:28:34Z

delegateBySig

Great suggestion. Made it working via this way, and it will be in 1 transaction as you proposed - together with the claim - as you and Keng proposed for.

This way the user pays and transact, no need to store it.

Anyways, if we were about to do with 2 transactions, then i would do the :

claim airdrop
delegate (by the user, non signature based, since it is easier on the ui).

Brechtpd

I think delegating with a signature is a good approach. Claiming for other addresses should still be allowed I feel so that users don't have to interact with each address that has an airdrop separately, but this would be off the default path so something they could just use etherscan for or something. The UI requiring the same address claiming the tokens to also sign and delegate seems like a good default flow.

Anyways, if we were about to do with 2 transactions, then i would do the :

Seems easy (and I think even necessary for general case) to support both cases.

Brechtpd · 2024-02-09T18:06:24Z

packages/protocol/contracts/team/airdrop/MerkleClaimable.sol

@@ -59,7 +76,15 @@ abstract contract MerkleClaimable is EssentialContract {
        }

        isClaimed[hash] = true;
-        _claimWithData(data);


Seems a bit like a strange setup where the claiming has an extra internal function to extend the functionality. More logical to me would be to have an external claimAndDelegate() function (or something like that) that would call claim and also calls delegateBySig().

I think from the op's perspective, we want to make sure if some one is in the airdrop list, the address must claim by itself, no way can claim for it, so we filter out dead addresses. And we also want to make sure whenever you claim, you must delegate to someone.

However, the extra function shall go to ERC20Aridrop contracts, not in this parent contract.

Brechtpd · 2024-02-09T18:09:36Z

packages/protocol/contracts/tokenvault/BridgedERC20.sol

+contract BridgedERC20 is
+    BridgedERC20Base,
+    IERC20MetadataUpgradeable,
+    ERC20SnapshotUpgradeable,
+    ERC20VotesUpgradeable


The downside of having to do one size fits all, now all bridged tokens need to support the onchain voting functionality because we want it for TKO (will be nicer in the next protocol version!). :)

Yes, my proposal was a special check in Bridge -> if TKO then mint into a modified contract.
Now every bridge contract can basically basically used in a DAO, which i'd say does not hurt, right ? Maybe it is a feature.. ?
Deployment size does not matter bc. we anyway pre-deploy the implementation. If snapsot and voting is not used frequently also the transfer is not 'heavy' (same cost) so i dont see it a bug but rather maybe a cool feature(?).
What do you see the downsizes or side effects of it ?
Taiko can even be a chain which boost your token and make "DAO-able" :D

Please see #15726

packages/protocol/contracts/tokenvault/BridgedERC20.sol

adaki2004 · 2024-02-10T03:06:06Z

packages/protocol/test/team/airdrop/SigUtil.sol

+                abi.encodePacked(
+                    "\x19\x01",
+                    DOMAIN_SEPARATOR,
+                    getStructHash(_permit)


It is exactly the same. It is just a helper sig util. The true "thing" is to re-create on the UI..!
We need to test it back and forth.

dantaik · 2024-02-10T03:08:01Z

packages/protocol/test/team/airdrop/SigUtil.sol

+// SPDX-License-Identifier: MIT
+pragma solidity 0.8.24;
+
+// Creating the hashTypedDataV4 hash type signing


OZ has library code to support EIP 712,

See:

import "@openzeppelin/contracts/utils/cryptography/draft-EIP712.sol"; contract MessageSigner is EIP712 { string private constant SIGNING_DOMAIN = "MessageSignerDomain"; string private constant SIGNATURE_VERSION = "1"; struct Message { address from; string message; } bytes32 private constant MESSAGE_TYPEHASH = keccak256( "Message(address from,string message)" ); function getMessageHash(Message memory message) public view returns (bytes32) { return _hashTypedDataV4(keccak256(abi.encode( MESSAGE_TYPEHASH, message.from, keccak256(bytes(message.message)) // Correctly hash the string message ))); } }

I also prefer to move the code to ERC20Airrop.sol or its parent contract.

dantaik · 2024-02-10T03:13:47Z

packages/protocol/test/team/airdrop/SigUtil.sol

+// SPDX-License-Identifier: MIT
+pragma solidity 0.8.24;
+
+// Creating the hashTypedDataV4 hash type signing


See:

import "@openzeppelin/contracts/utils/cryptography/draft-EIP712.sol"; contract MessageSigner is EIP712 { string private constant SIGNING_DOMAIN = "MessageSignerDomain"; string private constant SIGNATURE_VERSION = "1"; struct Message { address from; string message; } bytes32 private constant MESSAGE_TYPEHASH = keccak256( "Message(address from,string message)" ); function getMessageHash(Message memory message) public view returns (bytes32) { return _hashTypedDataV4(keccak256(abi.encode( MESSAGE_TYPEHASH, message.from, keccak256(bytes(message.message)) // Correctly hash the string message ))); } }

dantaik · 2024-02-10T03:15:31Z

packages/protocol/test/team/airdrop/SigUtil.sol

+// SPDX-License-Identifier: MIT
+pragma solidity 0.8.24;
+
+// Creating the hashTypedDataV4 hash type signing


I also prefer to move the code to ERC20Airrop.sol or its parent contract.

dantaik · 2024-02-10T03:17:10Z

packages/protocol/contracts/team/airdrop/ERC20Airdrop.sol

+        uint8 v,
+        bytes32 r,
+        bytes32 s
+    ) internal override {


If both ERC20Airdrop and ERC20Airdrop2 allow claiming, we shall create a parent contract and move shared code there.

dantaik · 2024-02-10T03:17:37Z

packages/protocol/contracts/team/airdrop/ERC721Airdrop.sol

@@ -40,7 +40,15 @@ contract ERC721Airdrop is MerkleClaimable {
        vault = _vault;
    }

-    function _claimWithData(bytes calldata data) internal override {
+    function _claimWithData(


ERC721Airdrop shall not be changed at all -- delegation shall not be something MerkleClaimable shall care.

Then how do you achieve the desired one transaction per airdrop ? I already told you and Keng i prefer the 2 time transactions, because of errors too for example (UI does not consturcts sig things properly, etc.)

dantaik · 2024-02-10T03:23:56Z

@adaki2004 I still think we should split snapshot from airdrop and have 2 PRs.

adaki2004 · 2024-02-10T03:29:09Z

@adaki2004 I still think we should split snapshot from airdrop and have 2 PRs.

Please proceed as you wish, i'll be OOF most of today and tomorrow- or wait till monday. Also would be good if the requirements would be more clear not changing from day to day.
First it was asked to bundle into one transaction, now it is possible (with built in sigantures), and you say now: ERC721Airdrop shall not be changed at all. -> Maybe a common base class (beside Merkle) to do the delegation ? Still the MerkleClaimable needs to be changes. (input params need to come from somewhere).

If requirements are not perfectly known (and changing) it creates wasted efforts like this and double work.

dantaik · 2024-02-10T05:41:11Z

@adaki2004 please review: #15727, if this PR is merged, I think you can revert all files except BridgedERC20.sol

dantaik · 2024-02-10T05:41:37Z

packages/protocol/contracts/tokenvault/BridgedERC20.sol

+contract BridgedERC20 is
+    BridgedERC20Base,
+    IERC20MetadataUpgradeable,
+    ERC20SnapshotUpgradeable,
+    ERC20VotesUpgradeable


Please see #15726

adaki2004 · 2024-02-10T14:40:42Z

Before the airdrop transaction, we collect a delegateSignature from the user, making sure its valid; then the user proceed with the airdrop claim function. Two signatures, one transaction.

Later, we asynchronously fire many delegateBySig transactions behind the scene. The misson will be accomlished this way.

We can also ask the user to transact his delegateBySig transaction after receiving the airdrop.

What if we collect and the signature will not be valid afterwards ?
Either expires, tho we can ask for a long time OR they delegate before we fire away and the signature nonce is not valid anymore ?

I guess it is not a huge issue as it means the user delegated on his own.

adaki2004 · 2024-02-10T15:07:30Z

Closing as we implemented it in 2 separate PRs. (Was easier to open up a new one with BridgedERC20 than revert some changes here.)

make BridgedERC20 delegatable and adjust airdrop tests

798469e

adaki2004 requested review from Brechtpd and dantaik February 9, 2024 13:15

Adjust BridgedERC20

7cfd7a0

dantaik reviewed Feb 9, 2024

View reviewed changes

packages/protocol/contracts/tokenvault/BridgedERC20.sol Outdated Show resolved Hide resolved

dantaik reviewed Feb 9, 2024

View reviewed changes

packages/protocol/contracts/tokenvault/BridgedERC20.sol Show resolved Hide resolved

use delegateBySig as Daniel proposed

039cb7a

adaki2004 requested a review from dantaik February 9, 2024 17:28

Brechtpd reviewed Feb 9, 2024

View reviewed changes

adaki2004 commented Feb 10, 2024

View reviewed changes

dantaik reviewed Feb 10, 2024

View reviewed changes

dantaik requested changes Feb 10, 2024

View reviewed changes

Merge branch 'main' into delegateable

c2ee32b

dantaik requested changes Feb 10, 2024

View reviewed changes

feat(protocol): add shapshooter for BridgedERC20 (#15726)

dbd0695

adaki2004 mentioned this pull request Feb 10, 2024

feat(protocol): Add votes and snapshot plugin #15732

Merged

adaki2004 closed this Feb 10, 2024

dantaik deleted the delegateable branch March 1, 2024 04:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(protocol): Add votes and snapshot plugins to BridgedERC20 #15715

feat(protocol): Add votes and snapshot plugins to BridgedERC20 #15715

adaki2004 commented Feb 9, 2024 •

edited

Loading

dantaik commented Feb 9, 2024

dantaik commented Feb 9, 2024 •

edited

Loading

adaki2004 commented Feb 9, 2024

adaki2004 commented Feb 9, 2024

Brechtpd left a comment •

edited

Loading

Brechtpd Feb 9, 2024

dantaik Feb 10, 2024

dantaik Feb 10, 2024

Brechtpd Feb 9, 2024

adaki2004 Feb 10, 2024

dantaik Feb 10, 2024

adaki2004 Feb 10, 2024 •

edited

Loading

dantaik Feb 10, 2024

dantaik Feb 10, 2024

dantaik Feb 10, 2024

dantaik Feb 10, 2024

dantaik Feb 10, 2024

dantaik Feb 10, 2024

dantaik Feb 10, 2024

adaki2004 Feb 10, 2024

dantaik commented Feb 10, 2024

adaki2004 commented Feb 10, 2024 •

edited

Loading

dantaik commented Feb 10, 2024

dantaik Feb 10, 2024

adaki2004 commented Feb 10, 2024

adaki2004 commented Feb 10, 2024

feat(protocol): Add votes and snapshot plugins to BridgedERC20 #15715

feat(protocol): Add votes and snapshot plugins to BridgedERC20 #15715

Conversation

adaki2004 commented Feb 9, 2024 • edited Loading

dantaik commented Feb 9, 2024

dantaik commented Feb 9, 2024 • edited Loading

adaki2004 commented Feb 9, 2024

adaki2004 commented Feb 9, 2024

Brechtpd left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adaki2004 Feb 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dantaik commented Feb 10, 2024

adaki2004 commented Feb 10, 2024 • edited Loading

dantaik commented Feb 10, 2024

Choose a reason for hiding this comment

adaki2004 commented Feb 10, 2024

adaki2004 commented Feb 10, 2024

adaki2004 commented Feb 9, 2024 •

edited

Loading

dantaik commented Feb 9, 2024 •

edited

Loading

Brechtpd left a comment •

edited

Loading

adaki2004 Feb 10, 2024 •

edited

Loading

adaki2004 commented Feb 10, 2024 •

edited

Loading