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.

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

Entity clarification #35

Merged
merged 13 commits into from
Mar 26, 2024
Merged

Entity clarification #35

merged 13 commits into from
Mar 26, 2024

Conversation

anihamde
Copy link
Contributor

@anihamde anihamde commented Mar 21, 2024

  • separate operator into relayer and admin
  • create onlyAdmin and onlyRelayer modifiers and apply them to methods
  • separate state variables into a distinct state contract inherited by ExpressRelay
  • pay out relayer fees atomically within multicall
  • refactor tests to separate unit tests from integration tests, using common setup methods
  • create MulticallData struct to enforce equal length of the fields in multicall

uint256 feeSplitRelayer
) {
// TODO: move this to ExpressRelayState?
state.feeSplitPrecision = 10 ** 18;
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this is gonna be constant, let's just make it a constant.


contract ExpressRelayStorage {
struct State {
// address of admin of express relay
Copy link
Collaborator

Choose a reason for hiding this comment

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

some of these comments do not help in understanding the fields. Comments here should explain what each variable "does" instead of explaining what each variable "is".

@@ -602,6 +608,22 @@ contract ExpressRelayIntegrationTest is
assertEq(balancePost, balancePre + totalBid);
}

function testSetRelayerByAdmin() public {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's move these to a different file. These are not integration tests anymore. I think we should have another test suite that only tests the express relay (actual unit tests) with some dummy contracts to call on top.

per_multicall/script/Vault.s.sol Show resolved Hide resolved
uint256 feeRelayerNumerator = (totalBid - feeProtocol) *
state.feeSplitRelayer;
if (feeRelayerNumerator > 0) {
uint256 feeRelayer = feeRelayerNumerator / state.feeSplitPrecision;
Copy link
Collaborator

Choose a reason for hiding this comment

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

why not calculate this first and check feeRelayer > 0 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

little bit of gas costs savings. in practice this is gonna be minimal, can just divide in same line

@anihamde anihamde marked this pull request as ready for review March 24, 2024 17:08

function testSetRelayerByAdmin() public {
address newRelayer = makeAddr("newRelayer");
vm.prank(admin, admin);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it necessary to set the second argument? We don't do any sort of checks based on tx.origin. Same for all the other pranks


function testSetRelayerByNonAdminFail() public {
address newRelayer = makeAddr("newRelayer");
vm.expectRevert(abi.encodeWithSelector(Unauthorized.selector));
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should work without the encodeWithSelector too. same for the rest of the tests

uint256 feeSplitProtocolDefaultPost = expressRelay
.getFeeProtocolDefault();

assertEq(feeSplitProtocolDefaultPre, feeSplitProtocolDefault);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This feeSplitProtocolDefault variable is a bit magical here. Also this assertion doesn't seem to be testing the setFeeProtocolDefault function

uint256 fee = feeSplitProtocolDefaultPre + 1;
vm.expectRevert(abi.encodeWithSelector(Unauthorized.selector));
vm.prank(relayer, relayer);
expressRelay.setFeeProtocolDefault(fee);
Copy link
Collaborator

Choose a reason for hiding this comment

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

we expect unauthorized here so you can just pass in 0 and simplify the test a bit.

expressRelay.setFeeProtocolDefault(fee);
}

function testSetFeeProtocolByAdmin() public {
Copy link
Collaborator

Choose a reason for hiding this comment

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

all the above comments apply to the other getter/setter tests too

state.admin = admin;
state.relayer = relayer;

if (feeSplitProtocolDefault > state.feeSplitPrecision) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this checking logic can become a separate function

assertEq(balancesAPost.collateral, balancesAPre.collateral);
assertEq(balancesAPost.debt, balancesAPre.debt);

assertFailedExternal(multicallStatuses[0], "InvalidLiquidation()");
Copy link
Collaborator

Choose a reason for hiding this comment

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

let's use InvalidLiquidation.selector here too.
This really helps with refactoring. My IDE was dumb and when I renamed one of these errors, it didn't automatically changed the string version.

*/
function setUpContracts() public {
// instantiate multicall contract with ExpressRelay operator as the deployer
// TODO: change admin to xc-admin
Copy link
Collaborator

Choose a reason for hiding this comment

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

what does this comment mean here?

/**
* @notice setUp function - sets up the contracts, wallets, tokens, oracle feeds, and vaults for the test
*
* This function creates the entire environment for the start of each test. It is called before each test.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is mostly a repetition of the comments on each of these functions which is unnecessary.

*
* ExpressRelayTestSetup also defines helper methods that are commonly used in the test suites.
*/
contract ExpressRelayTestSetup is
Copy link
Collaborator

Choose a reason for hiding this comment

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

Many of the functions here can still be splited into smaller functions. Not necessarily in this PR but some time in the future

--systemdf and others added 2 commits March 25, 2024 19:18
Copy link
Collaborator

@m30m m30m left a comment

Choose a reason for hiding this comment

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

Thanks for the refactor on the rust side, looks much better now. Please fix the comments before merging.

winner_bids.iter().map(|b| b.target_contract).collect(),
winner_bids.iter().map(|b| b.target_calldata.clone()).collect(),
winner_bids.iter().map(|b| b.bid_amount).collect(),
winner_bids.iter().map(|b| MulticallData{target_contract: b.target_contract, target_calldata: b.target_calldata.clone(), bid_amount: b.bid_amount }).collect()
Copy link
Collaborator

Choose a reason for hiding this comment

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

you can implement the From trait for MulticallData so that the conversion logic is just implemented once.

// split of the non-protocol fees to be paid to the relayer
uint256 feeSplitRelayer;
// precision for fee splits
uint256 feeSplitPrecision;
Copy link
Collaborator

Choose a reason for hiding this comment

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

what I mean is to remove it from the state and just have it as a constant in our contracts.

@anihamde anihamde merged commit 1815c6e into main Mar 26, 2024
1 check passed
@anihamde anihamde deleted the entity_clarification branch March 26, 2024 12:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants