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.

Sign up for GitHub

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

Jump to bottom

Chain configs #34

Merged
merged 3 commits into from
Aug 28, 2024
Merged

Chain configs #34

merged 3 commits into from
Aug 28, 2024

Conversation

coa-telos
Copy link

Fixes #issue_number

Description

Test scenarios

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have cleaned up the code in the areas my change touches
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules
  • I have checked my code and corrected any misspellings
  • I have removed any unnecessary console messages
  • I have included all english text to the translation file and/or created a new issue with the required translations for the currently supported languages
  • I have tested for mobile functionality and responsiveness
  • I have added appropriate test coverage
  • I have updated relevant documentation and/or opened a separate issue to cover the updates (Issue URL: )

@coa-telos coa-telos mentioned this pull request Aug 14, 2024
Closed
3 tasks
@coa-telos coa-telos force-pushed the issue-26-chain-configs branch from c75874f to 5a77fc8 Compare August 14, 2024 15:40
@coa-telos coa-telos marked this pull request as ready for review August 14, 2024 15:40
@coa-telos coa-telos force-pushed the issue-26-chain-configs branch 2 times, most recently from 48102e9 to 7c8b773 Compare August 14, 2024 15:48
poplexity
poplexity reviewed Aug 14, 2024
View reviewed changes
Copy link
Member

@poplexity poplexity left a comment

Choose a reason for hiding this comment

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

Looks good to me, left some comments that are generally focused on reducing the number of changes we make so we can reduce the chances of conflicts during future merges.

bin/reth/src/cli/mod.rs
@@ -271,6 +271,9 @@ mod tests {
/// Tests that the log directory is parsed correctly. It's always tied to the specific chain's
/// name
#[test]
#[cfg(not(feature = "telos"))]
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need to disable the upstream tests individually like this, there will be a lot of tests that will not work with our changes and we want to make as few changes as possible (to reduce merge conflicts). I have removed the .github directory and added our own CI workflow which will just run our E2E test.

bin/reth/src/optimism.rs
@@ -14,7 +14,7 @@ use std::sync::Arc;
#[global_allocator]
static ALLOC: tikv_jemallocator::Jemalloc = tikv_jemallocator::Jemalloc;

#[cfg(not(feature = "optimism"))]
#[cfg(all(not(feature = "optimism"), not(feature = "telos")))]
Copy link
Member

Choose a reason for hiding this comment

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

I see you also removed a similar change from bin/reth/src/main.rs, do we need either of these? If we always build with the telos feature flag, should we need to worry at all about the other binaries besides our own bin/reth/src/telos.rs? If possible, neither of these changes would be ideal, so we have fewer changes/merge conflicts.

Copy link
Author

Choose a reason for hiding this comment

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

not sure I'm following, you think that its OK to make it compilable only with our feture flag, and doing that way will reduce differences?

crates/chainspec/src/spec.rs Outdated
@@ -30,6 +30,8 @@ use reth_network_peers::{
base_nodes, base_testnet_nodes, holesky_nodes, mainnet_nodes, op_nodes, op_testnet_nodes,
sepolia_nodes,
};
#[cfg(feature = "telos")]
Copy link
Member

Choose a reason for hiding this comment

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

Initially we will not have the reth nodes doing any peering, the Telos node itself peers on the network and then reth is just consuming the blocks from that node. Possibly after do our initial release we will want to find a way to run reth without a Telos node, which could use peering, but we haven't planned for that yet.

So, if this is needed for reth to function then that's good, but if this is only needed to allow reth to peer with other reth nodes, we can skip this change for the initial release.

Copy link
Member

Choose a reason for hiding this comment

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

This would apply to all the other changes related to peers and bootnodes.

@poplexity poplexity force-pushed the telos-crate branch 3 times, most recently from 08de37b to 66469d3 Compare August 17, 2024 17:07
@poplexity poplexity force-pushed the issue-26-chain-configs branch from 7c8b773 to 52a0454 Compare August 17, 2024 20:21
@poplexity poplexity force-pushed the issue-26-chain-configs branch from 52a0454 to 71d0946 Compare August 17, 2024 20:23
@coa-telos coa-telos force-pushed the issue-26-chain-configs branch 3 times, most recently from e8c14c7 to 48e5034 Compare August 21, 2024 12:23
@coa-telos coa-telos force-pushed the issue-26-chain-configs branch from 48e5034 to cdfd02a Compare August 21, 2024 15:08
@poplexity poplexity merged commit 673f5e0 into telos-crate Aug 28, 2024
@poplexity poplexity deleted the issue-26-chain-configs branch August 28, 2024 23:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@poplexity poplexity poplexity left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@coa-telos @poplexity