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

feat(ETH transport & heartbeats): various enhancements/features #2058

Merged
merged 55 commits into from
Feb 26, 2024

Conversation

onur-ozkan
Copy link
Member

@onur-ozkan onur-ozkan commented Jan 23, 2024

  • ETH websocket transport
    • komodo-defi-proxy signed messages implementation
    • lock & wait free request pipelines for response quickness
    • expirable hashmap implementation (so request contexts can be cleaned in REQUEST_TIMEOUT_AS_SEC)
  • Refactor web3 contexts, connection rotation and remove singular web3 field from ETH coin
  • [ ] Implementing and using fn with_socket for ETH COIN_BALANCE event (3c2313a)
  • Heartbeats implementation for streaming channels.
  • Refactored node rotation (moved from transport to protocol level, so it can be rotated across different transports)
  • RPC abstraction for ETH

@onur-ozkan onur-ozkan added enhancement New feature or request in progress Changes will be made from the author labels Jan 23, 2024
@onur-ozkan onur-ozkan changed the title feat(transport, streaming channels): various features on ETH and streaming channels feat(ETH transport & heartbeats): various enhancements/features Jan 23, 2024
Signed-off-by: onur-ozkan <work@onurozkan.dev>
Signed-off-by: onur-ozkan <work@onurozkan.dev>
Signed-off-by: onur-ozkan <work@onurozkan.dev>
Signed-off-by: onur-ozkan <work@onurozkan.dev>
Signed-off-by: onur-ozkan <work@onurozkan.dev>
Signed-off-by: onur-ozkan <work@onurozkan.dev>
Signed-off-by: onur-ozkan <work@onurozkan.dev>
Signed-off-by: onur-ozkan <work@onurozkan.dev>
Signed-off-by: onur-ozkan <work@onurozkan.dev>
Signed-off-by: onur-ozkan <work@onurozkan.dev>
Signed-off-by: onur-ozkan <work@onurozkan.dev>
Signed-off-by: onur-ozkan <work@onurozkan.dev>
Signed-off-by: onur-ozkan <work@onurozkan.dev>
Signed-off-by: onur-ozkan <work@onurozkan.dev>
Signed-off-by: onur-ozkan <work@onurozkan.dev>
Signed-off-by: onur-ozkan <work@onurozkan.dev>
Signed-off-by: onur-ozkan <work@onurozkan.dev>
Signed-off-by: onur-ozkan <work@onurozkan.dev>
Signed-off-by: onur-ozkan <work@onurozkan.dev>
Signed-off-by: onur-ozkan <work@onurozkan.dev>
Signed-off-by: onur-ozkan <work@onurozkan.dev>
Signed-off-by: onur-ozkan <work@onurozkan.dev>
Signed-off-by: onur-ozkan <work@onurozkan.dev>
Signed-off-by: onur-ozkan <work@onurozkan.dev>
Copy link
Collaborator

@mariocynicys mariocynicys left a comment

Choose a reason for hiding this comment

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

Thanks!

Q: To get my thoughts straight, the websocket_transport.rs implementation is thread safe because we get a new id (hashmap key) each time thus we will never access (read/write) the same key from two threads at the same time?

mm2src/coins/eth/web3_transport/metamask_transport.rs Outdated Show resolved Hide resolved
mm2src/coins/eth/v2_activation.rs Outdated Show resolved Hide resolved
mm2src/coins/eth/v2_activation.rs Outdated Show resolved Hide resolved
mm2src/coins/eth.rs Show resolved Hide resolved
Web3Transport::Metamask(metamask) => metamask.execute(method, params.clone()),
};

match execute_fut.timeout(Duration::from_secs(15)).await {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you define this 15 literal as a const instead? Also, isn't 15 a very long time :o

Copy link
Member Author

@onur-ozkan onur-ozkan Feb 22, 2024

Choose a reason for hiding this comment

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

Made the Duration constant altogether and used in EthCoin::get_live_client and EthCoin::try_rpc_send.

let response_map = unsafe { &mut *self.responses.0 };
let _ = response_map.insert(request_id, res_bytes);

notifier.send(()).expect("receiver channel must be alive");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Won't this ever panic?

Copy link
Member Author

Choose a reason for hiding this comment

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

No. Caller is awaiting for the response through this channel. If it ever drop before that, we should panic as something def. going wrong.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ahaa I was thinking this was a notifier for external communication (not us).

shamardy
shamardy previously approved these changes Feb 22, 2024
Copy link
Collaborator

@shamardy shamardy left a comment

Choose a reason for hiding this comment

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

🔥

Signed-off-by: onur-ozkan <work@onurozkan.dev>
@onur-ozkan
Copy link
Member Author

Q: To get my thoughts straight, the websocket_transport.rs implementation is thread safe because we get a new id (hashmap key) each time thus we will never access (read/write) the same key from two threads at the same time?

That's part of it. Each request utilizes an atomic ID and only makes modifications using that ID. Additionally, we are waiting for notification from another thread to read and remove the key, ensuring that we don't attempt to read and delete it while another thread is attempting to write to it. This sequential computing is ensured via the thread channel notification.

laruh
laruh previously approved these changes Feb 23, 2024
Copy link
Member

@laruh laruh left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Collaborator

@mariocynicys mariocynicys left a comment

Choose a reason for hiding this comment

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

This sequential computing is ensured via the thread channel notification.

I get the point in that we process all the responses in a single worker thread, but don't we ever access the map from another thread (even with a different key)?

e.g. are we always sure ...

mm2src/coins/eth/web3_transport/websocket_transport.rs Outdated Show resolved Hide resolved
mm2src/coins/eth/web3_transport/websocket_transport.rs Outdated Show resolved Hide resolved
Signed-off-by: onur-ozkan <work@onurozkan.dev>
@onur-ozkan onur-ozkan linked an issue Feb 26, 2024 that may be closed by this pull request
mariocynicys
mariocynicys previously approved these changes Feb 26, 2024
Copy link
Collaborator

@mariocynicys mariocynicys left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks!

mm2src/coins/eth/web3_transport/websocket_transport.rs Outdated Show resolved Hide resolved
Signed-off-by: onur-ozkan <work@onurozkan.dev>
@shamardy shamardy merged commit 6aa5d66 into dev Feb 26, 2024
24 of 30 checks passed
@shamardy shamardy deleted the eth-ws-and-heartbeats branch February 26, 2024 13:51
dimxy added a commit to dimxy/komodo-defi-framework that referenced this pull request Mar 13, 2024
* dev:
  feat(indexeddb): advanced cursor filtering impl (KomodoPlatform#2066)
  update dockerhub destination repository (KomodoPlatform#2082)
  feat(event streaming): configurable worker path, use SharedWorker (KomodoPlatform#2080)
  fix(hd_tests): fix test_hd_utxo_tx_history unit test (KomodoPlatform#2078)
  feat(network): improve efficiency of known peers handling (KomodoPlatform#2074)
  feat(nft): enable eth with non fungible tokens (KomodoPlatform#2049)
  feat(ETH transport & heartbeats): various enhancements/features (KomodoPlatform#2058)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

heartbeats for event stream channels
5 participants