feat: Permissioned Across-v2 bond token #226

pxrl · 2023-02-01T15:38:37Z

This PR creates a new bond token, to be used as the collateral required by the HubPool on the following functions:

proposeRootBundle()
disputeRootBundle()

The new bond token (symbol "ABT") hosts an internal set of approved proposer addresses in order to impose constraints on attempted inbound ABT transfers to the HubPool. Permissioned addresses are unconstrained in their transfer of ABT, whereas all other addresses must not be the proposer of a root bundle in order to transfer ABT to the HubPool. This permits any ABT holder to submit a dispute of an existing root bundle proposal, whilst preventing non-approved addresses from proposing.

ABT inherits from the WETH9 contract with minimal modifications necessary to override the transferFrom() function.

Fixes ACX-699 ACX-700

This PR creates a new bond token, to be used as the collateral required by the HubPool on the following functions: - proposeRootBundle() - disputeRootBundle() The new bond token (symbol "ABT") hosts an internal set of approved proposer addresses in order to impose constraints on attempted inbound ABT transfers to the HubPool. Permissioned addresses are unconstrained in their transfer of ABT, whereas all other addresses must _not_ be the proposer of a root bundle in order to transfer ABT to the HubPool. This permits any ABT holder to submit a dispute of an existing root bundle proposal, whilst preventing non-approved addresses from proposing. ABT inherits from the WETH9 contract with minimal modifications necessary to override the transferFrom() function.

linear · 2023-02-01T15:38:40Z

ACX-699 Initial contract implementation

ACX-700 Test implementation

deploy/019_deploy_bond_token.ts

test/BondToken.e2e.ts

nicholaspai · 2023-02-01T16:16:37Z

contracts/BondToken.sol

+import "./HubPoolInterface.sol";
+import "./WETH9.sol";
+
+interface _HubPool is HubPoolInterface {


Prefer sticking to existing naming convention and not prefixing this with a _. Maybe name HubPoolExtendedInterface or something, or even IHubPool

Good point - I'll find something better 👍

Implemented: 91c1780

nicholaspai · 2023-02-01T16:17:13Z

contracts/BondToken.sol

+     * @param _hubPool Address of the target HubPool contract.
+     */
+    constructor(_HubPool _hubPool) {
+        name = "Across v2 Bond Token";


Suggested change

name = "Across v2 Bond Token";

name = "Across Bond Token";

Not sure V2 is necessary. Do you think we should name it Proposer Bond instead of just Bond?

Preface: Naming things is hard :)

My thinking with the "v2" naming is that this token has a finite lifespan - it will only be in use as long as the current (Across v2) HubPool is deployed (nb. the HubPool address is immutable, per current implementation). That's why I preferred to include v2 in the name.

I'd prefer not to include "Proposer" in the name, since this token is also needed to dispute.

I generally don't like Versions in token names, look at UMA Voting Token V1 for example

"v2" removed: ee0aedb

I do think it's a different scenario to UMA v1, but I can live with either :)

nicholaspai · 2023-02-01T16:17:38Z

contracts/BondToken.sol

+     */
+    constructor(_HubPool _hubPool) {
+        name = "Across v2 Bond Token";
+        symbol = "ABT";


nit: can we come up with a catchier symbol? :P

"AMT" => "Across Moon Token" ?

(Slightly) more seriously:
APRT => Across Proposer Responsibility Token
APPT => Across Permissioned Proposer Token
APDT => Across Permisionless Dispute Token
AHST => Across HubPool Security Token

IDK 🤷

Will think on this more haha

nicholaspai · 2023-02-01T16:19:10Z

contracts/BondToken.sol

+        uint256 amt
+    ) public override returns (bool) {
+        if (dst == address(hubPool)) {
+            require(proposers[src] || hubPool.rootBundleProposal().proposer != src, "Transfer not permitted");


Does this mean proposer's can't dispute themselves? I don't think we should block such behavior

This check only requires that one of the conditions is true, so approved proposers can always self-dispute without impediment (and, all other users can dispute as well). There's a test that verifies that - see test/BondToken.e2e.ts line 72.

The statement hubPool.rootBundleProposal().proposer != src does seem counter-intuitive at first. It's there based on the observation that when a non-approved proposer is attempting to propose, their address is recorded as rootBundleProposal.proposer.

This is extremely counterintuitive, but it's really clever. This is the sort of logic that I think Certora would be really good for verifying. We would probably want to verify that any address with tokens can always send a dispute.

I'm not super worried about this edge case, but this logic does seem to mean that if I proposed and then I was removed from the whitelist, I would then be no longer able to dispute myself.

Just out of curiousity, why did you not use the the check we use in the HubPool, that unclaimedPoolRebalanceLeafCount != 0 to detect disputes vs proposals. See the noActiveRequests modifier.

EDIT: That last idea doesn't work. I didn't quite get the nuance you picked up on, that the variables are written before the transfer. I think the way you've done it or checking the timestamp is probably the only way I could see this working. Very clever.

I'm not super worried about this edge case, but this logic does seem to mean that if I proposed and then I was removed from the whitelist, I would then be no longer able to dispute myself.

That was also my intuition, but then I tried to implement a test for it and found the opposite - it works unexpectedly. This is similarly because the pending root bundle is deleted before the bond token transferFrom() function is called, so the disputer is no longer the proposer, even though both the proposal and dispute were submitted by the same address. It's a happy accident, I guess.

For reference, see the test at line 78 in test/BondToken.e2e.ts.

Just out of curiousity, why did you not use the the check we use in the HubPool, that unclaimedPoolRebalanceLeafCount != 0 to detect disputes vs proposals. See the noActiveRequests modifier.

That was my first attempt as well, but it doesn't work (edit: As mentioned in your update 👍). At the time of evaluation, the proposed root bundle is the version submitted by the proposer, so it seems reliably non-zero in case of both proposals and disputes.

This is extremely counterintuitive

I've added a dev comment to the BondToken.safeTransferFrom() implementation to call this out more explicitly. See 5215b60.

I had the same question as Matt but see your point now!

Wow, it keeps getting weirder @pxrl. Really would love some validation on this!

contracts/WETH9.sol

nicholaspai

Did a once over and left some comments

contracts/BondToken.sol

Comment from Nick.

For compliance with GPLv3 sections 4. and 5: https://www.gnu.org/licenses/gpl-3.0.html

And relocate the pragma.

mrice32

This is a really clever implementation. Nice work, @pxrl!

mrice32 · 2023-02-01T21:41:47Z

contracts/BondToken.sol

+        uint256 amt
+    ) public override returns (bool) {
+        if (dst == address(hubPool)) {
+            require(proposers[src] || hubPool.rootBundleProposal().proposer != src, "Transfer not permitted");


This is extremely counterintuitive, but it's really clever. This is the sort of logic that I think Certora would be really good for verifying. We would probably want to verify that any address with tokens can always send a dispute.

I'm not super worried about this edge case, but this logic does seem to mean that if I proposed and then I was removed from the whitelist, I would then be no longer able to dispute myself.

Just out of curiousity, why did you not use the the check we use in the HubPool, that unclaimedPoolRebalanceLeafCount != 0 to detect disputes vs proposals. See the noActiveRequests modifier.

EDIT: That last idea doesn't work. I didn't quite get the nuance you picked up on, that the variables are written before the transfer. I think the way you've done it or checking the timestamp is probably the only way I could see this working. Very clever.

mrice32 · 2023-02-01T21:46:59Z

contracts/WETH9.sol

+        return transferFrom(msg.sender, dst, wad);
+    }
+
+    function transferFrom(


nit: can we add a comment that this is the only difference between this contract and the real WETH9?

Added here: edbee53.

mrice32 · 2023-02-01T21:48:35Z

contracts/WETH9.sol

+
+pragma solidity ^0.8.0;
+
+contract WETH9 {


nit: we probably don't want two contracts in the repo called WETH9 -- this causes issues in hardhat I think. Any reason we can't just delete the interface or rename it to WETH9Interface?

Good point - I'd totally missed the existing interface. I've split this out - see #227.

Make way a copy of the WETH9 implementation to be imported.

Import the full contract so that it can be tweaked and used as the base contract for the Across Bond Token (name subject to change). The import is made as-is, with no changes. Follow-up commits will apply modifications as required.

- Fix SPDX license identifier: AGPL-3.0-only => GPL-3.0-or-later - Reinstate WETH9 license header for compliance with GPLv3 sections 4. and 5: https://www.gnu.org/licenses/gpl-3.0.html - Relocate the pragma.

The base branch was changed.

…ndtoken

Signed-off-by: Paul <108695806+pxrl@users.noreply.github.com>

Note: We should remember to seed the HubPool with a tiny balance to save gas on each proposal.

mrice32

LGTM!

mrice32 · 2023-02-06T23:55:21Z

contracts/BondToken.sol

+        uint256 amt
+    ) public override returns (bool) {
+        if (dst == address(hubPool)) {
+            require(proposers[src] || hubPool.rootBundleProposal().proposer != src, "Transfer not permitted");


Wow, it keeps getting weirder @pxrl. Really would love some validation on this!

pxrl requested review from nicholaspai and mrice32 February 1, 2023 15:38

pxrl commented Feb 1, 2023

View reviewed changes

deploy/019_deploy_bond_token.ts Show resolved Hide resolved

pxrl commented Feb 1, 2023

View reviewed changes

test/BondToken.e2e.ts Show resolved Hide resolved

nicholaspai reviewed Feb 1, 2023

View reviewed changes

contracts/WETH9.sol Outdated Show resolved Hide resolved

nicholaspai reviewed Feb 1, 2023

View reviewed changes

pxrl commented Feb 1, 2023

View reviewed changes

contracts/BondToken.sol Outdated Show resolved Hide resolved

pxrl added 5 commits February 1, 2023 21:52

improve: _HubPool => ExtendedHubPoolInterface

91c1780

Comment from Nick.

improve: Cleanup test imports

50c957d

improve: Clarify WETH9 provenance

551f8d8

fix: Reinstate WETH9 license header

2dfd014

For compliance with GPLv3 sections 4. and 5: https://www.gnu.org/licenses/gpl-3.0.html

fix: SPDX license identifier

992a479

And relocate the pragma.

mrice32 previously approved these changes Feb 1, 2023

View reviewed changes

pxrl added 3 commits February 1, 2023 23:40

chore: Rename WETH9.sol => WETH9Interface.sol

4213b70

Make way a copy of the WETH9 implementation to be imported.

chore: Import WETH9 from UMAproject/protocol

fc4e98e

Import the full contract so that it can be tweaked and used as the base contract for the Across Bond Token (name subject to change). The import is made as-is, with no changes. Follow-up commits will apply modifications as required.

fix: Clarify WETH9 provenance

6830850

- Fix SPDX license identifier: AGPL-3.0-only => GPL-3.0-or-later - Reinstate WETH9 license header for compliance with GPLv3 sections 4. and 5: https://www.gnu.org/licenses/gpl-3.0.html - Relocate the pragma.

pxrl changed the base branch from master to pxrl/weth9-shuffle February 1, 2023 22:45

pxrl added 6 commits February 1, 2023 23:58

Merge remote-tracking branch 'origin/pxrl/weth9-shuffle' into pxrl/bo…

be0b346

…ndtoken

fix: Reinstate WETH9 changes

edbee53

improve: Clarify transferFrom() require() assumptions

5215b60

Signed-off-by: Paul <108695806+pxrl@users.noreply.github.com>

improve: Simplify BondToken fixture setup

b1096e8

improve: Test proposer refunds

d860e61

fix: BondToken deployment script issues

2ae7164

fix: Cleanup hre dependencies

a6bad25

Base automatically changed from pxrl/weth9-shuffle to master February 2, 2023 21:34

pxrl added 4 commits February 2, 2023 22:36

Merge branch 'master' into pxrl/bondtoken

571b883

improve: Add basic gas tests for transferFrom()

104ec28

Note: We should remember to seed the HubPool with a tiny balance to save gas on each proposal.

Rename BondToken.Transfer => BondToken.erc20

b6a2598

Remove v2 from token name

ee0aedb

pxrl requested a review from mrice32 February 6, 2023 23:42

fix test

f4de94c

mrice32 approved these changes Feb 6, 2023

View reviewed changes

pxrl merged commit b320ba7 into master Feb 7, 2023

pxrl deleted the pxrl/bondtoken branch February 7, 2023 09:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Permissioned Across-v2 bond token #226

feat: Permissioned Across-v2 bond token #226

pxrl commented Feb 1, 2023

linear bot commented Feb 1, 2023

nicholaspai Feb 1, 2023

pxrl Feb 1, 2023

pxrl Feb 1, 2023

nicholaspai Feb 1, 2023

pxrl Feb 1, 2023 •

edited

Loading

nicholaspai Feb 3, 2023

pxrl Feb 6, 2023

nicholaspai Feb 1, 2023

pxrl Feb 1, 2023 •

edited

Loading

nicholaspai Feb 3, 2023

nicholaspai Feb 1, 2023

pxrl Feb 1, 2023 •

edited

Loading

mrice32 Feb 1, 2023 •

edited

Loading

pxrl Feb 1, 2023 •

edited

Loading

pxrl Feb 1, 2023 •

edited

Loading

nicholaspai Feb 3, 2023

mrice32 Feb 6, 2023

nicholaspai left a comment

mrice32 left a comment

mrice32 Feb 1, 2023 •

edited

Loading

mrice32 Feb 1, 2023

pxrl Feb 1, 2023

mrice32 Feb 1, 2023

pxrl Feb 1, 2023

mrice32 left a comment

mrice32 Feb 6, 2023

feat: Permissioned Across-v2 bond token #226

feat: Permissioned Across-v2 bond token #226

Conversation

pxrl commented Feb 1, 2023

linear bot commented Feb 1, 2023

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

pxrl Feb 1, 2023 • 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

pxrl Feb 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pxrl Feb 1, 2023 • edited Loading

Choose a reason for hiding this comment

mrice32 Feb 1, 2023 • edited Loading

Choose a reason for hiding this comment

pxrl Feb 1, 2023 • edited Loading

Choose a reason for hiding this comment

pxrl Feb 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicholaspai left a comment

Choose a reason for hiding this comment

mrice32 left a comment

Choose a reason for hiding this comment

mrice32 Feb 1, 2023 • 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

mrice32 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pxrl Feb 1, 2023 •

edited

Loading

pxrl Feb 1, 2023 •

edited

Loading

pxrl Feb 1, 2023 •

edited

Loading

mrice32 Feb 1, 2023 •

edited

Loading

pxrl Feb 1, 2023 •

edited

Loading

pxrl Feb 1, 2023 •

edited

Loading

mrice32 Feb 1, 2023 •

edited

Loading