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

feat: removed external dependencies from miden-objects #1186

Merged
merged 2 commits into from
Mar 3, 2025

Conversation

varun-doshi
Copy link
Contributor

Ref #1145

removed external dependencies from miden-objects

Copy link
Contributor

@PhilippGackstatter PhilippGackstatter 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! I think we can add a couple of re-exports and then this is ready.

prettier::PrettyPrint,
Program,
};
use assembly::Compile;
Copy link
Contributor

Choose a reason for hiding this comment

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

We can add a re-export of Compile in lib.rs and then import it from crate as well.

Program,
};
use assembly::Compile;
use vm_core::prettier::PrettyPrint;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can also add a re-export of PrettyPrint.

AdviceMap, Program,
};
use vm_processor::{AdviceInputs, DeserializationError};
use vm_core::mast::{MastForest, MastNodeId};
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's also add a re-export of MastForest and MastNodeId.

use super::{
Asset, ByteReader, ByteWriter, Deserializable, DeserializationError, Digest, Felt, Hasher,
NoteError, Serializable, Word, WORD_SIZE, ZERO,
use super::Asset;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you replace this Asset import with an import from crate? Thanks!

Copy link
Contributor

@PhilippGackstatter PhilippGackstatter 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!

Copy link
Contributor

@bobbinth bobbinth 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! Thank you!

@bobbinth bobbinth added the no changelog This PR does not require an entry in the `CHANGELOG.md` file label Mar 3, 2025
@bobbinth
Copy link
Contributor

bobbinth commented Mar 3, 2025

@varun-doshi - could you fix the lints? After this, we should be good to merge.

@bobbinth bobbinth merged commit a3b7e65 into 0xPolygonMiden:next Mar 3, 2025
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no changelog This PR does not require an entry in the `CHANGELOG.md` file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants