Skip to content
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.

Sign up for GitHub

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

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

Closed
wants to merge 5 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
798469e
make BridgedERC20 delegatable and adjust airdrop tests
Feb 9, 2024
7cfd7a0
Adjust BridgedERC20
Feb 9, 2024
039cb7a
use delegateBySig as Daniel proposed
Feb 9, 2024
c2ee32b
Merge branch 'main' into delegateable
dantaik Feb 10, 2024
dbd0695
feat(protocol): add shapshooter for BridgedERC20 (#15726)
dantaik Feb 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion packages/protocol/contracts/team/airdrop/ERC20Airdrop.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,19 @@ contract ERC20Airdrop is MerkleClaimable {
vault = _vault;
}

function _claimWithData(bytes calldata data) internal override {
function _claimWithData(
bytes calldata data,
address delegatee,
uint256 nonce,
uint256 expiry,
uint8 v,
bytes32 r,
bytes32 s
) internal override {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

(address user, uint256 amount) = abi.decode(data, (address, uint256));
// Transfer the tokens
IERC20(token).transferFrom(vault, user, amount);
// Do the delegation transaction safely by the user
Delegation(token).delegateBySig(delegatee, nonce, expiry, v, r, s);
}
}
10 changes: 9 additions & 1 deletion packages/protocol/contracts/team/airdrop/ERC20Airdrop2.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,15 @@ contract ERC20Airdrop2 is MerkleClaimable {
withdrawableAmount = timeBasedAllowance - withdrawnAmount[user];
}

function _claimWithData(bytes calldata data) internal override {
function _claimWithData(
bytes calldata data,
address /*delegatee*/,
uint256 /*nonce*/,
uint256 /*expiry*/,
uint8 /*v*/,
bytes32 /*r*/,
bytes32 /*s*/
) internal override {
(address user, uint256 amount) = abi.decode(data, (address, uint256));
claimedAmount[user] += amount;
}
Expand Down
10 changes: 9 additions & 1 deletion packages/protocol/contracts/team/airdrop/ERC721Airdrop.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,15 @@ contract ERC721Airdrop is MerkleClaimable {
vault = _vault;
}

function _claimWithData(bytes calldata data) internal override {
function _claimWithData(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

bytes calldata data,
address /*delegatee*/,
uint256 /*nonce*/,
uint256 /*expiry*/,
uint8 /*v*/,
bytes32 /*r*/,
bytes32 /*s*/
) internal override {
(address user, uint256[] memory tokenIds) = abi.decode(data, (address, uint256[]));

for (uint256 i; i < tokenIds.length; ++i) {
Expand Down
39 changes: 36 additions & 3 deletions packages/protocol/contracts/team/airdrop/MerkleClaimable.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,17 @@ import { MerkleProofUpgradeable } from
"lib/openzeppelin-contracts-upgradeable/contracts/utils/cryptography/MerkleProofUpgradeable.sol";
import "../../common/EssentialContract.sol";

interface Delegation {
function delegateBySig(
address delegatee,
uint256 nonce,
uint256 expiry,
uint8 v,
bytes32 r,
bytes32 s
) external;
}

/// @title MerkleClaimable
/// Contract for managing Taiko token airdrop for eligible users
abstract contract MerkleClaimable is EssentialContract {
Expand All @@ -44,7 +55,13 @@ abstract contract MerkleClaimable is EssentialContract {

function claim(
bytes calldata data,
bytes32[] calldata proof
bytes32[] calldata proof,
address delegatee,
uint256 nonce,
uint256 expiry,
uint8 v,
bytes32 r,
bytes32 s
)
external
nonReentrant
Expand All @@ -59,7 +76,15 @@ abstract contract MerkleClaimable is EssentialContract {
}

isClaimed[hash] = true;
_claimWithData(data);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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().

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

_claimWithData(
data,
delegatee,
nonce,
expiry,
v,
r,
s
);
emit Claimed(hash);
}

Expand All @@ -85,5 +110,13 @@ abstract contract MerkleClaimable is EssentialContract {
}

/// @dev Must revert in case of errors.
function _claimWithData(bytes calldata data) internal virtual;
function _claimWithData(
bytes calldata data,
address delegate,
uint256 nonce,
uint256 expiry,
uint8 v,
bytes32 r,
bytes32 s
) internal virtual;
}
61 changes: 55 additions & 6 deletions packages/protocol/contracts/tokenvault/BridgedERC20.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,22 @@ pragma solidity 0.8.24;
import
"lib/openzeppelin-contracts-upgradeable/contracts/token/ERC20/extensions/IERC20MetadataUpgradeable.sol";
import "lib/openzeppelin-contracts/contracts/utils/Strings.sol";

import
"lib/openzeppelin-contracts-upgradeable/contracts/token/ERC20/extensions/ERC20SnapshotUpgradeable.sol";
import
"lib/openzeppelin-contracts-upgradeable/contracts/token/ERC20/extensions/ERC20VotesUpgradeable.sol";
import "./LibBridgedToken.sol";
import "./BridgedERC20Base.sol";

/// @title BridgedERC20
/// @notice An upgradeable ERC20 contract that represents tokens bridged from
/// another chain.
contract BridgedERC20 is BridgedERC20Base, IERC20MetadataUpgradeable, ERC20Upgradeable {
contract BridgedERC20 is
BridgedERC20Base,
IERC20MetadataUpgradeable,
ERC20SnapshotUpgradeable,
ERC20VotesUpgradeable
Comment on lines +30 to +34
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!). :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see #15726

{
address public srcToken; // slot 1
uint8 private srcDecimals;
uint256 public srcChainId; // slot 2
Expand Down Expand Up @@ -65,13 +73,21 @@ contract BridgedERC20 is BridgedERC20Base, IERC20MetadataUpgradeable, ERC20Upgra
// Initialize OwnerUUPSUpgradable and ERC20Upgradeable
__Essential_init(_addressManager);
__ERC20_init({ name_: _name, symbol_: _symbol });
__ERC20Snapshot_init();
__ERC20Votes_init();
__ERC20Permit_init(_name);

// Set contract properties
srcToken = _srcToken;
srcChainId = _srcChainId;
srcDecimals = _decimals;
}

/// @notice Creates a new token snapshot.
function snapshot() public onlyOwner {
adaki2004 marked this conversation as resolved.
Show resolved Hide resolved
_snapshot();
}

/// @notice Gets the name of the token.
/// @return The name.
function name()
Expand Down Expand Up @@ -119,16 +135,49 @@ contract BridgedERC20 is BridgedERC20Base, IERC20MetadataUpgradeable, ERC20Upgra
_burn(from, amount);
}

/// @dev For ERC20SnapshotUpgradeable and ERC20VotesUpgradeable, need to implement the following
/// functions
function _beforeTokenTransfer(
address, /*from*/
address from,
address to,
uint256 /*amount*/
uint256 amount
)
internal
virtual
override
override(ERC20Upgradeable, ERC20SnapshotUpgradeable)
adaki2004 marked this conversation as resolved.
Show resolved Hide resolved
{
if (to == address(this)) revert BTOKEN_CANNOT_RECEIVE();
if (paused()) revert INVALID_PAUSE_STATUS();
super._beforeTokenTransfer(from, to, amount);
}

function _afterTokenTransfer(
address from,
address to,
uint256 amount
)
internal
override(ERC20Upgradeable, ERC20VotesUpgradeable)
{
super._afterTokenTransfer(from, to, amount);
}

function _mint(
address to,
uint256 amount
)
internal
override(ERC20Upgradeable, ERC20VotesUpgradeable)
{
super._mint(to, amount);
}

function _burn(
address from,
uint256 amount
)
internal
override(ERC20Upgradeable, ERC20VotesUpgradeable)
{
super._burn(from, amount);
}
}
Loading
Loading