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

fix(naming): couple of naming improvements #27

Merged
merged 19 commits into from
Mar 13, 2024
Merged

fix(naming): couple of naming improvements #27

merged 19 commits into from
Mar 13, 2024

Conversation

m30m
Copy link
Collaborator

@m30m m30m commented Mar 8, 2024

No description provided.

@@ -1,7 +1,7 @@
chains:
development:
geth_rpc_addr: http://localhost:8545
per_contract: 0xa513E6E4b8f2a923D98304ec87F64353C4D5C853
relay_contract: 0xa513E6E4b8f2a923D98304ec87F64353C4D5C853
Copy link
Contributor

Choose a reason for hiding this comment

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

rename to express_relay_contract

Copy link
Contributor

Choose a reason for hiding this comment

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

or rename all instances of express relay contract to relay contract. basically just maintain consistency

#[arg(env = "PER_PRIVATE_KEY")]
pub per_private_key: String,
/// A 20-byte (40 char) hex encoded Ethereum private key which is the express relay operator.
#[arg(long = "relay-private-key")]
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe relay-operator-private-key? would be clearer

@@ -1,4 +1,4 @@
# PER
# Express Relay

## [Off-chain server](auction-server/README.md)
Copy link
Contributor

Choose a reason for hiding this comment

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

let's rename the whole per_multicall folder (and the mentions thereof in this README) to contracts or solidity? that folder is just on-chain contract code and it would be good to make this naming reflect that

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This folder is gonna be open-sourced and moved to the other monorepo in a few months. I'd rather keep this as is and then move it with a better naming and cleaner to the main repo.

1. Edit `config.yaml` to point to the desired blockchains and PER contracts. You can use `config.sample.yaml` as a template.
2. Generate a secret key to be used for the PER operator. The PER contract should be deployed with this address as the operator.
1. Edit `config.yaml` to point to the desired blockchains and Express Relay contracts. You can use `config.sample.yaml` as a template.
2. Generate a secret key to be used for the Express Relay operator. The Express Relay contract should be deployed with this address as the operator.
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use the term "relayer" everywhere as opposed to "operator". I know we've been somewhat inconsistent about this in the code and docs, but would be good to standardize.

@@ -1,7 +1,7 @@
chains:
development:
geth_rpc_addr: http://localhost:8545
per_contract: 0xa513E6E4b8f2a923D98304ec87F64353C4D5C853
relay_contract: 0xa513E6E4b8f2a923D98304ec87F64353C4D5C853
adapter_contract: 0xB7f8BC63BbcaD18155201308C8f3540b07f84F5e
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use "adapter" or "opportunity adapter" instead of "liquidation adapter" everywhere. will send you more thoughts in slack

address[] calldata contracts,
bytes[] calldata data,
uint256[] calldata bids
uint256[] calldata bidAmounts
Copy link
Contributor

Choose a reason for hiding this comment

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

this is good, we should make this clarification btwn bids and bidAmounts everywhere it pops up. otherwise very confusing

@@ -58,51 +58,51 @@ contract LiquidationAdapter is SigVerify {
function callLiquidation(
Copy link
Contributor

Choose a reason for hiding this comment

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

callTarget

@@ -58,51 +58,51 @@ contract LiquidationAdapter is SigVerify {
function callLiquidation(
LiquidationCallParams memory params
Copy link
Contributor

Choose a reason for hiding this comment

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

TargetCallParams

@@ -656,7 +662,7 @@ contract PERIntegrationTest is
}

function testLiquidateSingle() public {
// test PER path liquidation (via multicall, per operator calls) with searcher contract
// test ExpressRelay path liquidation (via multicall, express relay operator calls) with searcher contract
Copy link
Contributor

Choose a reason for hiding this comment

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

via entrypoint contract

@@ -25,8 +25,8 @@ contract Signatures is Test, SigVerify {
}

function createLiquidationSignature(
Copy link
Contributor

Choose a reason for hiding this comment

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

createAdapterSignature?

@@ -32,9 +32,9 @@ To run a happy path test of the on-chain contracts plus the off-chain services,
f. Retrieve the number saved under "searcherAOwnerSk", convert to a hex string, and save as SEARCHER_SK. You can perform this conversion in Python by calling hex on the number.
Copy link
Contributor

Choose a reason for hiding this comment

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

can we rename the perOperator above --> perRelayer?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm creating another PR changing this e2e test and the docs along with it. Will be fixed there.

@@ -112,7 +112,7 @@ pub async fn post_opportunity(
bidders: Default::default(),
};

verify_opportunity(params.clone(), chain_store, store.per_operator.address())
verify_opportunity(params.clone(), chain_store, store.relayer.address())
.await
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 this file and all instances of "liquidation" can be renamed to something along the lines of "opportunity". i think the only part of "liquidation" that should remain is the endpoint path

@@ -90,20 +91,21 @@ impl TryFrom<EthereumConfig> for Provider<Http> {
}

pub fn get_simulation_call(
per_operator: Address,
relayer: Address,
provider: Provider<Http>,
chain_config: EthereumConfig,
permission: Bytes,
Copy link
Contributor

Choose a reason for hiding this comment

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

are we standardizing this as permission_key?

provider,
chain_config,
permission,
contracts,
target_contracts,
calldata,
Copy link
Contributor

Choose a reason for hiding this comment

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

the naming here is bit confusing since calldata is both singular and plural... is target_calldata any clearer that this corresponds to the target_contracts?

@@ -186,7 +186,7 @@ pub async fn submit_bids(
permission: Bytes,
contracts: Vec<Address>,
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this be target_contracts?

// Signature: 0xd3c8346c
error LiquidationCallFailed(string reason);
// Signature: 0xf00364b1
error ExecutionFailed(string reason);
Copy link
Contributor

Choose a reason for hiding this comment

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

ExternalExecutionFailed? just for verbosity to clarify the execution failed at the target contract

bytes calldata permission,
address[] calldata contracts,
bytes calldata permissionKey,
address[] calldata targetContracts,
bytes[] calldata data,
Copy link
Contributor

Choose a reason for hiding this comment

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

similar to above, targetData?

@@ -18,7 +18,7 @@ struct Bid {
contract AuctionManager {
Copy link
Contributor

Choose a reason for hiding this comment

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

is this file worth maintaining? probably can get rid of this

Copy link
Contributor

Choose a reason for hiding this comment

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

can add admin role and responsibilities to ExpressRelay.sol

TokenAmount[] sellTokens;
TokenAmount[] buyTokens;
address executor;
address target;
Copy link
Contributor

Choose a reason for hiding this comment

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

smallest of nits: targetContract? a bit clearer

@@ -98,8 +102,10 @@ contract VaultScript is Script {
vm.startBroadcast(skDeployer);
if (pyth == address(0)) pyth = deployMockPyth();
if (weth == address(0)) weth = deployWeth();
(address per, address liquidationAdapter) = deployPER(weth);
address vault = deployVault(per, pyth);
(address expressRelay, address liquidationAdapter) = deployExpressRelay(
Copy link
Contributor

Choose a reason for hiding this comment

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

rename liquidationAdapter--fine if you want to do this with the changes to the tests later

/// Token amount
#[schema(example = "1000", value_type=String)]
#[serde(with = "crate::serde::u256")]
pub amount: U256,
pub amount: U256,
}

/// Opportunity parameters needed for on-chain execution
/// If a searcher signs the opportunity and have approved enough tokens to liquidation adapter,
Copy link
Contributor

Choose a reason for hiding this comment

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

remove reference to liquidation

}

/// Opportunity parameters needed for on-chain execution
/// If a searcher signs the opportunity and have approved enough tokens to liquidation adapter,
/// by calling this contract with the given calldata and structures, they will receive the tokens specified
/// in the receipt_tokens field, and will send the tokens specified in the repay_tokens field.
/// in the buy_tokens field, and will send the tokens specified in the sell_tokens field.
#[derive(Serialize, Deserialize, ToSchema, Clone, PartialEq)]
pub struct OpportunityParamsV1 {
/// The permission key required for succesful execution of the liquidation.
Copy link
Contributor

Choose a reason for hiding this comment

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

liquidation

#[schema(example = "sepolia", value_type=String)]
pub chain_id: ChainId,
/// The contract address to call for execution of the liquidation.
/// The contract address to call for execution of the opportunity.
#[schema(example = "0xcA11bde05977b3631167028862bE2a173976CA11", value_type=String)]
pub contract: ethers::abi::Address,
Copy link
Contributor

Choose a reason for hiding this comment

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

what do you think about making this target_contract?

Copy link
Contributor

Choose a reason for hiding this comment

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

and calldata --> target_calldata? just for the sake of verbosity


Tests can be found in `test/`. These tests include checks that the protocol functions work, as well as checks around permissioning, bid conditions, and appropriate failing of components of the multicall bundle (without failing the whole bundle).
Tests can be found in `test/`. These tests include checks that the protocol functions work, as well as checks around permissioning, bid conditions, and appropriate failing of components of the express relay bid bundle (without failing the whole bundle).
Copy link
Contributor

Choose a reason for hiding this comment

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

express relay bundle

@m30m m30m merged commit ef211c7 into main Mar 13, 2024
1 check passed
@m30m m30m deleted the renames branch March 13, 2024 10:00
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