Skip to content

Conversation

4rjunc
Copy link

@4rjunc 4rjunc commented Aug 7, 2025

PR for #16

I have pushed code of simple implementation of using tape.toml . In the following code change of only rpc_url is used from tape.toml config file. Couple of changes I made

  • add toml crate for handling toml file
  • removed default cluster option of l from cli args
  • priority is given to cli commands. if cluster is mention in cli command, rpc_url from tape.toml wont be used. if not it will be used.
  • if config is loaded from tape.toml it is passed through cli context
  • tape.toml is created at home dir

I tested the above change in archive command

~/tape.toml

[solana]
rpc_url = "https://api.devnet.solana.com"
ws_url = "wss://api.devnet.solana.com/"
commitment = "confirmed"
  • cargo run -p tapedrive-cli --manifest-path ~/Documents/solana/tape/cli/Cargo.toml archive
    Screenshot 2025-08-07 at 1 35 20 PM

  • cargo run -p tapedrive-cli --manifest-path ~/Documents/solana/tape/cli/Cargo.toml archive -ul
    Screenshot 2025-08-07 at 1 35 53 PM

@Nagaprasadvr
Copy link
Contributor

Cool ,
can u disable rust formatting, this repo is not using rust formatting and the pr will add unnecessary changes

@Nagaprasadvr
Copy link
Contributor

Also i feel we dont wanna use cli args because the reason for adding toml is to make things easier to configure wdyt @zfedoran

@4rjunc
Copy link
Author

4rjunc commented Aug 7, 2025

add rustfmt.toml to disable auto formatting. dropped the commits with auto formatted code. redone the changes to last two commits.

@zfedoran
Copy link
Collaborator

zfedoran commented Aug 7, 2025

Welcome @4rjunc, I hope this is one of many PRs, I'd love to see more in the future.

I'm curious, why are we removing the local cluster option?

@4rjunc
Copy link
Author

4rjunc commented Aug 7, 2025

I'm curious, why are we removing the local cluster option

I removed the default local cluster option because tape.toml is generated on first run with a default rpc_url (devnet). The CLI checks for an rpc_url in the options first if defaults exist, it won't use the config value.

@zfedoran
Copy link
Collaborator

zfedoran commented Aug 7, 2025

Ah understood, I thought the l option was being removed entirely, not just the default.

@4rjunc
Copy link
Author

4rjunc commented Aug 9, 2025

Also i feel we dont wanna use cli args because the reason for adding toml is to make things easier to configure wdyt @zfedoran

Quoting this again in case you missed replying @zfedoran

Copy link
Contributor

@Nagaprasadvr Nagaprasadvr left a comment

Choose a reason for hiding this comment

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

few things to update , can u also add a unit test to test the toml parsing works properly

}

/// validate values in tape.toml
fn validate(&self) -> anyhow::Result<()> {
Copy link
Contributor

@Nagaprasadvr Nagaprasadvr Aug 22, 2025

Choose a reason for hiding this comment

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

we also need to check urls are right here, just check for valid url using grep or something

@zfedoran
Copy link
Collaborator

So there is a non-zero chance that we may point tapedrive at all three clusters at the same time, but only one would be considered primary (where tokens are earned). Does this remove the need for the config file?

I guess not, but it might be worth thinking about what this file would look like with more than one source.

@Nagaprasadvr what are your thoughts?

@Nagaprasadvr
Copy link
Contributor

Nagaprasadvr commented Aug 23, 2025

So there is a non-zero chance that we may point tapedrive at all three clusters at the same time, but only one would be considered primary (where tokens are earned). Does this remove the need for the config file?

I guess not, but it might be worth thinking about what this file would look like with more than one source.

@Nagaprasadvr what are your thoughts?

For that we can just change the cluster url when we spawn the node in the config file , ig along with this we can have a field primary_cluster to mark which cluster is primary to use

@Nagaprasadvr
Copy link
Contributor

So there is a non-zero chance that we may point tapedrive at all three clusters at the same time, but only one would be considered primary (where tokens are earned). Does this remove the need for the config file?

I guess not, but it might be worth thinking about what this file would look like with more than one source.

@Nagaprasadvr what are your thoughts?

U mean simultaneously running tape node on multiple clusters ?

@zfedoran

@zfedoran
Copy link
Collaborator

So there is a non-zero chance that we may point tapedrive at all three clusters at the same time, but only one would be considered primary (where tokens are earned). Does this remove the need for the config file?
I guess not, but it might be worth thinking about what this file would look like with more than one source.
@Nagaprasadvr what are your thoughts?

U mean simultaneously running tape node on multiple clusters ?

@zfedoran

Yeah, exactly this. The premise is that users can write wherever it is cheapest. Assuming they are okay with lower recovery guarantees.

@Nagaprasadvr
Copy link
Contributor

Nagaprasadvr commented Aug 24, 2025

So there is a non-zero chance that we may point tapedrive at all three clusters at the same time, but only one would be considered primary (where tokens are earned). Does this remove the need for the config file?
I guess not, but it might be worth thinking about what this file would look like with more than one source.
@Nagaprasadvr what are your thoughts?

U mean simultaneously running tape node on multiple clusters ?
@zfedoran

Yeah, exactly this. The premise is that users can write wherever it is cheapest. Assuming they are okay with lower recovery guarantees.

What is the difference will be in this case between a primary cluster ,(possibly mainnet ) and other clusters like devnet and testnet

@4rjunc
Copy link
Author

4rjunc commented Aug 26, 2025

@Nagaprasadvr I have pushed code addressing your comments. lmk if this needs to be changed. If this looks good to go, I will start working on code to pass values from tape.toml.

Copy link
Contributor

@Nagaprasadvr Nagaprasadvr left a comment

Choose a reason for hiding this comment

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

looking good

@zfedoran
Copy link
Collaborator

Lots of good stuff in here.

@4rjunc if you'd like, I'd love to give you access to the dev channel in discord

@4rjunc
Copy link
Author

4rjunc commented Aug 27, 2025

thank you!

@zfedoran i haven't joined the discord yet. i will join now. my username: 4rjunc

@4rjunc 4rjunc force-pushed the toml branch 2 times, most recently from 6b1229a to 496b5dc Compare September 1, 2025 18:20
Copy link
Contributor

@Nagaprasadvr Nagaprasadvr left a comment

Choose a reason for hiding this comment

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

Few things to fix

cli/src/main.rs Outdated

let context = Context::try_build(&cli)?;
let config = match TapeConfig::load_with_path(&cli.config_path) {
Copy link
Contributor

@Nagaprasadvr Nagaprasadvr Sep 2, 2025

Choose a reason for hiding this comment

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

Let's just do this

let config = TapeConfig::load(&cli.config)?;

Move this inside context so we can get everything from context

@zfedoran
Copy link
Collaborator

zfedoran commented Sep 2, 2025

This is looking really awesome @4rjunc. One more thing I'd love to see in here is the miner name. The current approach uses the word "default", which is fine, but we might as well put it in the config.

@Nagaprasadvr what are your thoughts?

(related issue: #36)

@Nagaprasadvr
Copy link
Contributor

This is looking really awesome @4rjunc. One more thing I'd love to see in here is the miner name. The current approach uses the word "default", which is fine, but we might as well put it in the config.

@Nagaprasadvr what are your thoughts?

(related issue: #36)

Yeah definitely ig i missed it but it's a good addition

@4rjunc
Copy link
Author

4rjunc commented Sep 2, 2025

This is looking really awesome @4rjunc. One more thing I'd love to see in here is the miner name. The current approach uses the word "default", which is fine, but we might as well put it in the config.

@Nagaprasadvr what are your thoughts?

(related issue: #36)

thank you!

you mean to use a custom name instead of "default" in this line right ?

@Nagaprasadvr
Copy link
Contributor

Nagaprasadvr commented Sep 3, 2025

This is looking really awesome @4rjunc. One more thing I'd love to see in here is the miner name. The current approach uses the word "default", which is fine, but we might as well put it in the config.
@Nagaprasadvr what are your thoughts?
(related issue: #36)

thank you!

you mean to use a custom name instead of "default" in this line right ?

Let's add miner_name to miner config and hardcode it to "default"

@Nagaprasadvr
Copy link
Contributor

Hey @4rjunc ig we have changes from previous commits did u rebase this ?

@zfedoran
Copy link
Collaborator

zfedoran commented Sep 3, 2025

This is looking really awesome @4rjunc. One more thing I'd love to see in here is the miner name. The current approach uses the word "default", which is fine, but we might as well put it in the config.
@Nagaprasadvr what are your thoughts?
(related issue: #36)

thank you!

you mean to use a custom name instead of "default" in this line right ?

Yes, exactly this

> transaction
> performance
> identity
> solana
> storage
> network
> logging

feat: add validate + default values for toml.config

feat: log_level is enum

fix: resolving comments

> change the `max_memory_mb` 2gb -> 16 gb
> add few comments on what to change

fix: remove the `NetworkConfig`

feat: add enum for commitment level

> add a impl for `CommitmentLevel`

fix: fix  the keypair loading up from ~ path

feat: removed the helper function for tilde, add function used in anchor

feat: add abstract StorageConfig

feat: add URL validation

fix: rename performance -> mining_config

> add max_poa_thread
> add max_pow_thread

feat: better error handling for config::load()

- ConfigFileNotFound
- InvalidUrl(String)
- KeypairNotFound(String)
- HomeDirectoryNotFound
- FileReadError(std::io::Error)
- ParseError(toml::de::Error)
- DefaultConfigCreationFailed(String)

feat: add `--config` option

feat: add error handling for config load

> create new config is not found in default path, `--config` is not used
> if config file not found in given path -> error will be shown

feat: add `--config` option

feat: add error handling for config load

> create new config is not found in default path, `--config` is not used
> if config file not found in given path -> error will be shown
Copy link
Contributor

@Nagaprasadvr Nagaprasadvr left a comment

Choose a reason for hiding this comment

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

Few nits

> rm keypair_path
> update keypair_path() helper function
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.

3 participants