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

Add averageBlockTimeMs to all chains #45

Merged
merged 4 commits into from
Jul 6, 2023

Conversation

andreogle
Copy link
Member

Changes

  1. Adds averageBlockTimeMs to all chains. I've enforced this value be greater than 0 for now, so CI won't pass until each chain has a value.

@andreogle andreogle added the enhancement New feature or request label Jul 4, 2023
@andreogle andreogle requested review from bdrhn9 and vponline July 4, 2023 13:04
@andreogle andreogle self-assigned this Jul 4, 2023
@bbenligiray
Copy link
Member

Considering that additional similar fields will be added, I think this should be under an object, maybe called characteristics (parameters or properties didn't feel right because the existing fields could arguably go under these)

"averageBlockTimeMs": 0 is weird and may cause the user to make false assumptions. Should we make this optional and have an L2 flag instead?

Naming-wise, averageBlockTimeMs still isn't very exact, as when the averaging was done and for how long of a time it was done for are critical. So an exact name would be mostRecentMonthlyAverageBlockTimeMs, which is both ridiculous and not even true for the current values (because I assume the numbers are more than a month old). If the user is going to have to refer to the README for what the numbers mean anyway, I think it can be a bit more brief, as in blockTimeMs.

@andreogle
Copy link
Member Author

Considering that additional similar fields will be added, I think this should be under an object, maybe called characteristics

I have no objections to this. What about attributes instead?

"averageBlockTimeMs": 0

The 0 is temporary until the real values are filled in. I think we have these now , they just need to be added here

I think it can be a bit more brief, as in blockTimeMs

Happy with this too

@vponline
Copy link
Contributor

vponline commented Jul 5, 2023

I've added the values and the script which I used to calculate them. The script could be used in a Github Action to update them automatically if you like it.

@andreogle
Copy link
Member Author

I'm a bit unsure about constantly changing these values through an action or something similar. I think it's probably better to control them through manually running the script and then releasing a new minor version if we want new values. This can be discussed separately though

@bbenligiray
Copy link
Member

I'm fine with this. I think we need a sort of an L2 flag because the block time on an L1 and L2 are completely different things and depending applications would use these numbers differently (in general L2 block times are usage-dependent so the numbers here don't provide any guarantees so are meaningless)

@andreogle
Copy link
Member Author

Are you suggesting something like this?

{
  "characteristics": { // or "attributes"
    "blockTimeMs": 1234,
    "chainType": "layer-1" // or "layer-2"
  }
}

One problem I have is that it's not obvious to me what should belong in this nested object and what shouldn't

@bbenligiray
Copy link
Member

I think attributes doesn't work as a name because for example the testnet field to me is an attribute, semantically speaking. Maybe the more of these kinds of fields we have, we understand their common characteristics better. So we can add blockTimeMs (as is done in this PR) and the rest to the root for the time being and nest them in an object later on (which would be a breaking change but eh).

As a note, the main difference between these chains in the context of block time is not that some are L1s and some are L2s, but rather some are rollups with centralized sequencers, which are typically trusted with including the transactions in the sequence that they receive them in. These kind of chains also don't increment block numbers if there are no incoming transactions, which makes waiting for minimum transactions doubly meaningless (and we're planning to use block times to determine how many confirmations we'll wait). That being said, at the point that we have a rollup with a decentralized sequencer, the implementations that don't wait for confirmations if "rollup": true" will start misbehaving. So in what aspects we should describe a chain is a difficult question and we shouldn't rush it before gathering more data regarding how everyone is using them.

@andreogle andreogle merged commit e11d3fd into main Jul 6, 2023
3 checks passed
@andreogle andreogle deleted the feature/add-average-block-time branch July 6, 2023 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants