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

[bridge 4/n] SuiSyncer #15201

Merged
merged 1 commit into from
Dec 5, 2023
Merged

[bridge 4/n] SuiSyncer #15201

merged 1 commit into from
Dec 5, 2023

Conversation

longbowlu
Copy link
Contributor

@longbowlu longbowlu commented Dec 5, 2023

Description

Implements SuiSyncer that periodically pulls from Sui to fetch Bridge related events.

Test Plan

How did you test the new or updated feature?


If your changes are not user-facing and not a breaking change, you can skip the following section. Otherwise, please indicate what changed, and then add to the Release Notes section as highlighted during the release process.

Type of Change (Check all that apply)

  • protocol change
  • user-visible impact
  • breaking change for a client SDKs
  • breaking change for FNs (FN binary must upgrade)
  • breaking change for validators or node operators (must upgrade binaries)
  • breaking change for on-chain data layout
  • necessitate either a data wipe or data migration

Release notes

Implements SuiSyncer that periodically pulls from Sui to fetch Bridge related events.

Copy link

vercel bot commented Dec 5, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
mysten-ui ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 5, 2023 9:29pm
sui-core ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 5, 2023 9:29pm
sui-typescript-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 5, 2023 9:29pm
3 Ignored Deployments
Name Status Preview Comments Updated (UTC)
explorer ⬜️ Ignored (Inspect) Visit Preview Dec 5, 2023 9:29pm
multisig-toolkit ⬜️ Ignored (Inspect) Visit Preview Dec 5, 2023 9:29pm
sui-kiosk ⬜️ Ignored (Inspect) Visit Preview Dec 5, 2023 9:29pm

Copy link
Member

@mwtian mwtian left a comment

Choose a reason for hiding this comment

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

A few comments about eth and sui syncer,

  • It seems ethers is being deprecated in favor of alloy: https://docs.rs/ethers/latest/ethers/. Does this affect the usages here?
  • eth and sui syncer module can crash when fullnodes are disconnected. I'm not sure if it is better than retrying forever. E.g. even if the eth fullnode is down, in theory we should still be able to use the sui->eth quorum driver module?
  • A metric for last_finalized_block in eth syncer could be useful.


/// Map from contract address to their start block.
pub type SuiTargetModules = HashMap<Identifier, TransactionDigest>;
Copy link
Member

Choose a reason for hiding this comment

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

For a new contract, what would be the TransactionDigest?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it could be any transaction digest that happens before the new contract's existence - it's not very intuitive, i hope we have checkpoint as the cursor

sui_client.query_events_by_module(PACKAGE_ID, module.clone(), next_cursor),
Duration::from_secs(10)
)
.expect("Failed to query events from sui client after retry");
Copy link
Member

Choose a reason for hiding this comment

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

What is the reason for crashing instead of retrying (e.g. via continue) here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we retried with retry_with_max_delay a few lines above with 10s max time out. Do you think we should indefinitely retry to perhaps increase the max timeout latency?

Copy link
Member

Choose a reason for hiding this comment

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

Ok to use a larger timeout for now.

@longbowlu
Copy link
Contributor Author

longbowlu commented Dec 5, 2023

A few comments about eth and sui syncer,

The deprecation notice was up for two days when i chose the crate. I had a very brief look at alloy and feel it's not mature enough for us to adopt (may have some unnecessary hiccups. Some comments in gakonst/ethers-rs#2667 alloy have similar concerns. Ethers also have better docs.

  • eth and sui syncer module can crash when fullnodes are disconnected. I'm not sure if it is better than retrying forever. E.g. even if the eth fullnode is down, in theory we should still be able to use the sui->eth quorum driver module?

I need to think more about it. Initially I wanted to panic because it's easier to spot the disconnectivity. Is it ok that we use a larger timeout latency for now and add a TODO to change this behavior if needed? @mwtian

  • A metric for last_finalized_block in eth syncer could be useful.
    agree, will add a TODO


async fn run_event_listening_task(
// The module where interested events are defined.
// Moudle is always of bridge package 0x9.
Copy link
Contributor

Choose a reason for hiding this comment

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

Where are we adding a new package?

interval.tick().await;
// TODO reconsider panic, should we just log an error and continue the loop?
let events = retry_with_max_delay!(
sui_client.query_events_by_module(PACKAGE_ID, module.clone(), next_cursor),
Copy link
Contributor

Choose a reason for hiding this comment

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

For query_events_by_module, do you know if it's returning events emitted in the module code, or emitted when calling an entry function in the module code?

.expect("All Sui event channel receivers are closed");
// Unwrap: `query_events_by_module` always returns Some `next_cursor`
// If the events list is empty, `next_cursor` will be the same as `start_tx_digest`
next_cursor = events.next_cursor.unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

Still a bit fragile to depend on a fullnode behavior. We should not unwrap if it's trivial not to.

@longbowlu longbowlu merged commit de3e2ef into main Dec 5, 2023
39 checks passed
@longbowlu longbowlu deleted the bridge-4-sui-syncer branch December 5, 2023 21:46
gdanezis pushed a commit that referenced this pull request Dec 15, 2023
## Description 

Implements `SuiSyncer` that periodically pulls from Sui to fetch Bridge
related events.

## Test Plan 

How did you test the new or updated feature?

---
If your changes are not user-facing and not a breaking change, you can
skip the following section. Otherwise, please indicate what changed, and
then add to the Release Notes section as highlighted during the release
process.

### Type of Change (Check all that apply)

- [ ] protocol change
- [ ] user-visible impact
- [ ] breaking change for a client SDKs
- [ ] breaking change for FNs (FN binary must upgrade)
- [ ] breaking change for validators or node operators (must upgrade
binaries)
- [ ] breaking change for on-chain data layout
- [ ] necessitate either a data wipe or data migration

### Release notes
Implements `SuiSyncer` that periodically pulls from Sui to fetch Bridge
related events.
@zerosnacks
Copy link

Hi @longbowlu, happy to announce Alloy 0.1 has been released today!

https://www.paradigm.xyz/2024/06/alloy-release

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.

4 participants