Skip to content
This repository has been archived by the owner on Jun 24, 2024. It is now read-only.

docs: Hermes relayer data requirements #57

Merged
merged 20 commits into from
Apr 7, 2024

Conversation

Farhad-Shabani
Copy link
Member

@Farhad-Shabani Farhad-Shabani commented Jan 19, 2024

📖 Rendered

Based on: Hermes Data Requirements

Closes: #30
Part of: #32


PR author checklist

  • Linked to GitHub issue.
  • Added tests.
  • Updated code comments and documentation (e.g., docs/).
  • Tagged one reviewer who will be the one responsible for shepherding this PR.

@Farhad-Shabani Farhad-Shabani marked this pull request as ready for review January 19, 2024 03:07
docs/DataRequirements.md Show resolved Hide resolved
docs/DataRequirements.md Outdated Show resolved Hide resolved

## Sequencer RPC

### `/send_transactions`
Copy link
Collaborator

Choose a reason for hiding this comment

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

The sequencer_acceptTx RPC method on the sequencer does not seem to validate or return anything. This probably means there is no tx hash returned.

There is a separate sequencer_publishBatch method, which would submit all pending transactions as a blob to the DA. However it also only return a string that reports how many transactions have been published. This would probably be an internal method of the sequencer, as it doesn't make sense if any user can trigger the publishing at any time if the sequencer is exposed to public.

One possibility is that we can ask to get the tx hash of the blob submission to the Celestia DA. If we know which blob contains the transaction, we can probably trace it back to the rollup based on the blob hash.

Ideally though, all those should be keep tracked of on the rollup side. The sequencer needs to provide a tx hash for both the rollup tx and the blob tx, which can be traced back from the local rollup node of the relayer operator.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Found the calculation of the tx hash to be done here: https://github.com/Sovereign-Labs/sovereign-sdk/blob/nightly/full-node/sov-sequencer/src/batch_builder.rs#L163-L167

However the sequencer only prints the hash to the log and not return anything. On the other hand, it might be possible for us to deterministically get the tx hash without the sequencer, since it is really a hash of the tx raw bytes.

On the other hand, here it shows that if there are failure in the tx at the time of blob inclusion, it would just get silently discarded: https://github.com/Sovereign-Labs/sovereign-sdk/blob/nightly/full-node/sov-sequencer/src/batch_builder.rs#L147-L150

We probably need the sequencer to have an API to poll or subscribe to the status of the pending transactions, until the blob is submitted (or ideally be committed) to the DA.

Copy link
Collaborator

@ancazamfir ancazamfir left a comment

Choose a reason for hiding this comment

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

Great work here!
I propose we do a sync review meeting, remove anything that is not absolutely necessary in phase 1.
We also need an ADR for the relayer where we describe how it operates with zk rollups sovereign-sdk/celestia chain.

docs/DataRequirements.md Outdated Show resolved Hide resolved
docs/DataRequirements.md Outdated Show resolved Hide resolved
docs/DataRequirements.md Outdated Show resolved Hide resolved
docs/DataRequirements.md Outdated Show resolved Hide resolved
docs/DataRequirements.md Outdated Show resolved Hide resolved
docs/DataRequirements.md Outdated Show resolved Hide resolved
docs/DataRequirements.md Outdated Show resolved Hide resolved
docs/DataRequirements.md Outdated Show resolved Hide resolved
docs/DataRequirements.md Outdated Show resolved Hide resolved
docs/DataRequirements.md Outdated Show resolved Hide resolved
method to check the health of the RPC server.

## Rollup RPC

Copy link
Collaborator

Choose a reason for hiding this comment

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

We will probably need some /query_account RPC to get the account info.
In Cosmos chains this returns:

pub struct BaseAccount {
    #[prost(string, tag = "1")]
    pub address: ::prost::alloc::string::String,
    #[prost(message, optional, tag = "2")]
    pub pub_key: ::core::option::Option<super::super::super::google::protobuf::Any>,
    #[prost(uint64, tag = "3")]
    pub account_number: u64,
    #[prost(uint64, tag = "4")]
    pub sequence: u64,
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not an RPC but we also need other information like the address type, the public and private key generation, etc.
See *_key_pair.rs file in https://github.com/informalsystems/hermes/tree/master/crates/relayer/src/keyring used for Cosmos, Solana, Ethermint

Copy link
Member Author

Choose a reason for hiding this comment

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

Added: 4a5146e

As for the /query_account, there is an accounts_getAccount RPC endpoint which appears to perform the same job. However, it accepts a public key of the C::PublicKey and returns the C::Address type. Meaning that the concrete account type depends on the C (rollup context) that would be defined by the SDK implementors. For now, we can rely on their default ready-to-go definitions for such types. Therefore, We've got the Address, DefaultPublicKey, and DefaultPrivateKey of the ed25519 type.

@Farhad-Shabani
Copy link
Member Author

@ancazamfir, can we merge this for the first phase, and then apply any further updates needed with subsequent PRs?

@Farhad-Shabani
Copy link
Member Author

Merging this PR and as discussed, the status and reference to each required RPC method will be tracked in #126.

@Farhad-Shabani Farhad-Shabani merged commit e7dbad5 into main Apr 7, 2024
1 check passed
@Farhad-Shabani Farhad-Shabani deleted the farhad/data-requirements branch April 7, 2024 08:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

Investigate Hermes requirements from Sovereign SDK (Rollup)
3 participants