0xPolygonMiden / miden-client

Client library that facilitates interaction with the Miden rollup
MIT License
25 stars 22 forks source link

refactor: library api reorganization #367

Closed mFragaBA closed 2 weeks ago

mFragaBA commented 1 month ago

closes #362. closes #284.

I tried to have each commit as self contained as possible:

Discussion

tomyrd commented 1 month ago

The changes in miden_tx broke some of the imports here, I already commited some fixes to the parent branch so we only have to merge them.

mFragaBA commented 1 month ago

The changes in miden_tx broke some of the imports here, I already commited some fixes to the parent branch so we only have to merge them.

done! Rebased and pushed

mFragaBA commented 1 month ago
  • InvalidNoteInputsError seems like an internal-only error which doesn't appear in the public interface. If so, we should hide it (e.g., use pub(crate)).

While it does not appear in the public interface directly, it is used in NoteScreenerError which is part of the public api. Making InvalidNoteInputsError results in the following warning:

 1  warning: type `InvalidNoteInputsError` is more private than the item `NoteScreenerE
 rror::InvalidNoteInputsError::0`
    --> src/errors.rs:398:28
     |
 398 |     InvalidNoteInputsError(InvalidNoteInputsError),
     |                            ^^^^^^^^^^^^^^^^^^^^^^ field `NoteScreenerError::Inva
 lidNoteInputsError::0` is reachable at visibility `pub`
     |
 note: but type `InvalidNoteInputsError` is only usable at visibility `pub(crate)`
    --> src/errors.rs:428:1
     |
 428 | pub(crate) enum InvalidNoteInputsError {
     | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     = note: `#[warn(private_interfaces)]` on by default
bobbinth commented 1 month ago

While it does not appear in the public interface directly, it is used in NoteScreenerError which is part of the public api. Making InvalidNoteInputsError results in the following warning:

Ah - I see! Let's keep it as is then.

bobbinth commented 4 weeks ago

Is this ready for another review? Or should I hold off for now?

mFragaBA commented 4 weeks ago

Is this ready for another review? Or should I hold off for now?

should be, yes. I haven't been able to update the PR description to include the newer commits, but looking at the specific commit changes should be pretty self explanatory. Also opened #374 to refactor the errors