Skip to content

feat(sdk): some SDK improvements - #1568

Closed
PatStiles wants to merge 19 commits into
stagingfrom
feat/sdk-changes
Closed

feat(sdk): some SDK improvements#1568
PatStiles wants to merge 19 commits into
stagingfrom
feat/sdk-changes

Conversation

@PatStiles

@PatStilesPatStiles commented Dec 4, 2024

Copy link
Copy Markdown
Contributor

SDK Changes

Description

Makes some changes to sdk:

  • Derives batcher_url from Network paramater.
  • Adds --private_key flag to DepositToBatcher
  • Adds msg to batcher errors telling a user there funds have not been spent.

Type of change

Please delete options that are not relevant.

  • New feature
  • Bug fix
  • Optimization
  • Refactor

Checklist

  • “Hotfix” to testnet, everything else to staging
  • Linked to Github Issue
  • This change depends on code or research by an external entity
    • Acknowledgements were updated to give credit
  • Unit tests added
  • This change requires new documentation.
    • Documentation has been added/updated.
  • This change is an Optimization
    • Benchmarks added/run
  • Has a known issue
  • If your PR changes the Operator compatibility (Ex: Upgrade prover versions)
    • This PR adds compatibility for operator for both versions and do not change batcher/docs/examples
    • This PR updates batcher and docs/examples to the newer version. This requires the operator are already updated to be compatible

uri-99
uri-99 reviewed Dec 4, 2024
View reviewed changes
Comment threadbatcher/aligned/src/main.rs Outdated
uri-99
uri-99 reviewed Dec 4, 2024
View reviewed changes
Comment threadbatcher/aligned/src/main.rs
uri-99
uri-99 reviewed Dec 4, 2024
View reviewed changes
Comment threadbatcher/aligned/src/main.rs Outdated
uri-99
uri-99 reviewed Dec 4, 2024
View reviewed changes
Comment threadbatcher/aligned/src/main.rs Outdated
uri-99
uri-99 reviewed Dec 4, 2024
View reviewed changes
Comment threadbatcher/aligned/src/main.rs Outdated
uri-99
uri-99 reviewed Dec 4, 2024
View reviewed changes
Comment threadbatcher/aligned/src/main.rs
uri-99
uri-99 reviewed Dec 4, 2024
View reviewed changes
Comment threadbatcher/aligned/src/main.rs Outdated
uri-99
uri-99 suggested changes Dec 4, 2024
View reviewed changes

@uri-99uri-99 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

left some comments

@MarcosNicolau

MarcosNicolau commented Dec 4, 2024

Copy link
Copy Markdown
Collaborator

@uri-99, your comments have been addressed at: 2e8465c.

@uri-99uri-99 changed the title feat(sdk): SDK changesfeat(sdk): deprecata batcher_url parameterDec 5, 2024
@MarcosNicolauMarcosNicolau changed the title feat(sdk): deprecata batcher_url parameterfeat(sdk): deprecate batcher_url parameterDec 5, 2024
uri-99
uri-99 suggested changes Dec 5, 2024
View reviewed changes

@uri-99uri-99 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

missing remove batcher-url from rust task sender.

@uri-99uri-99 changed the title feat(sdk): deprecate batcher_url parameterfeat(sdk): some SDK improvementsJan 2, 2025
JulianVentura
JulianVentura approved these changes Jan 2, 2025
View reviewed changes

@JulianVenturaJulianVentura left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Works as expected!
There are some docs conflicts

avilagaston9
avilagaston9 reviewed Jan 6, 2025
View reviewed changes
Comment threadbatcher/aligned/src/main.rs
Comment on lines +472 to +475
if private_key.is_some() {
error!("Conflicting inputs detected: Both a keystore and a private key were provided. Provide only one option to proceed.");
return Ok(());
}

@avilagaston9avilagaston9Jan 6, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We could have used a group for this: https://stackoverflow.com/questions/76315540/how-do-i-require-one-of-the-two-clap-options

MauroToscano
MauroToscano requested changes Jan 6, 2025
View reviewed changes

@MauroToscanoMauroToscano left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This PR should be splitted in different PRs. Also, default network params is fine, but we should have a way to change the batcher url manually, let's re review this over the following days

This was referenced Jan 8, 2025
Closed
Merged
Merged
@JuArce
JuArce deleted the feat/sdk-changes branch March 28, 2025 18:35
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MarcosNicolauMarcosNicolauMarcosNicolau left review comments
@MauroToscanoMauroToscanoMauroToscano requested changes
+4 more reviewers
@avilagaston9avilagaston9avilagaston9 left review comments
@OppenOppenOppen approved these changes
@uri-99uri-99uri-99 approved these changes
@JulianVenturaJulianVenturaJulianVentura approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@PatStilesPatStiles

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@PatStiles@MarcosNicolau@Oppen@MauroToscano@uri-99@JulianVentura@avilagaston9