Fix OKX integration approve transaction #104
Conversation
|
Was tested on |
src/infra/dex/okx/mod.rs
Outdated
| let existing_dex_contract_address = self | ||
| .dex_approved_addresses | ||
| .write() | ||
| .await | ||
| .get(&order.sell) | ||
| .cloned(); | ||
|
|
||
| let dex_contract_address = match existing_dex_contract_address { | ||
| Some(value) => value, | ||
| None => { | ||
| let query_approve_transaction = | ||
| dto::ApproveTransactionRequest::with_domain(self.defaults.chain_id, order); | ||
|
|
||
| let approve_transaction: dto::ApproveTransactionResponse = self | ||
| .send("approve-transaction", &query_approve_transaction) | ||
| .instrument(tracing::trace_span!("approve_transaction", id = %id)) | ||
| .await?; | ||
|
|
||
| let address = eth::ContractAddress(approve_transaction.dex_contract_address); | ||
|
|
||
| self.dex_approved_addresses | ||
| .write() | ||
| .await | ||
| .put(order.sell, address); |
There was a problem hiding this comment.
The cache update needs to be done atomically, doesn't it? This will prevent sending duplicated approve-transaction calls. If so, blocking the whole collection should be avoided and a thread-safe LRU cache with segmentation blocking should be considered(moka).
src/infra/dex/okx/mod.rs
Outdated
| }) | ||
| } | ||
|
|
||
| async fn send<T, U>(&self, endpoint: &str, query: &T) -> Result<U, Error> |
There was a problem hiding this comment.
nit: The function signature can be improved to clearly state what it really does without requiring the reader to dive deeper into the implementation details.
| async fn send<T, U>(&self, endpoint: &str, query: &T) -> Result<U, Error> | |
| async fn send_get_request<T, U>(&self, api_path: &str, query: &T) -> Result<U, Error> |
src/infra/dex/okx/mod.rs
Outdated
| dto::ApproveTransactionRequest::with_domain(self.defaults.chain_id, order); | ||
|
|
||
| let approve_transaction: dto::ApproveTransactionResponse = self | ||
| .send("approve-transaction", &query_approve_transaction) |
There was a problem hiding this comment.
Ideally we'd be sending the /approve and /swap requests in parallel to reduce latency. Will not implement that in this PR to bring it to prod earlier.
| self.quote(&query) | ||
|
|
||
| let quote: dto::SwapResponse = self | ||
| .send("swap", &query) | ||
| .instrument(tracing::trace_span!("quote", id = %id)) |
There was a problem hiding this comment.
nit: instead of instrumenting the /swap and /approve call task separately they could just be put into the same function which gets instrumented as a whole. Would likely also help with readability.
Description
Added implementation for getting OKX smart contract address which is used during swap of a particular token handled by the OKX dex. This address needs to be used to set allowances (returned in
dex::Swap::allowance).Implementation
OKX smart contract address is taken from
/approve-transactionendpoint (documentation) for particular sell token. From my observations I saw at least two different smart contract addresses returned by this endpoint (for different tokens, for one token always same address was returned).Added generic function
send()which handles preparation, http roundtrip and errors handling for different OKX API calls.Map of: token address <=> OKX smart contract address for that token is stored in non-persistent LRU cache (size of 1000 items). Rust interior mutability pattern was used to simplify the implementation over the whole application (only changes to the okx module was needed).
Tests
Updated tests with additional endpoint requests mocks, added assert to api call test which verifies that allowance address.