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

Fix minor wallet issues #220

Merged
merged 21 commits into from
Oct 30, 2024
Merged

Fix minor wallet issues #220

merged 21 commits into from
Oct 30, 2024

Conversation

zees-dev
Copy link
Contributor

@zees-dev zees-dev commented Oct 29, 2024

Changelog

  • added tempfile to create test dirs for testing
    • updated test functions and util functions accordingly (removed redundant helper functions and replaced references)
  • added rust-toolchain.toml with stable rust 1.80.0
    • minor refactors to CI workflows to match the rust-toolchain.toml rust version
  • git ignoring IDE directory (.vscode/)
  • fixed new_at_index function to print checksum wallet address (currently printing bech32 address to stdout)
  • updated ensure_no_wallet_exists function to handle the following cases:
  • added unit tests for ensure_no_wallet_exists

@zees-dev zees-dev requested a review from a team October 29, 2024 12:54
@zees-dev zees-dev changed the title Fix/minor wallet issues Fix minor wallet issues Oct 29, 2024
rust-toolchain.toml Outdated Show resolved Hide resolved
Copy link
Member

@kayagokalp kayagokalp left a comment

Choose a reason for hiding this comment

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

Left a comment about rust-toolchain because I don't think it is doing what we are expecting to do, let's discuss first before merging

@zees-dev zees-dev requested a review from a team October 29, 2024 22:42
kayagokalp
kayagokalp previously approved these changes Oct 29, 2024
Copy link
Member

@kayagokalp kayagokalp left a comment

Choose a reason for hiding this comment

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

Nice! Thanks for cleaning those little but frustrating issues 🚀 I left couple of final nits. A final note would be to splitting this type of PRs into more focused ones (as we squash-merge this commit and the message of it won't be very descriptive).

For this PR this is fine to merge as it is as the diff is very small but in general it might be better to separate things based on what they do as we are not rebase-merging or merging via merge commit.

rust-toolchain.toml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@kayagokalp kayagokalp added bug Something isn't working testing Everything to do with testing labels Oct 29, 2024
src/utils.rs Outdated Show resolved Hide resolved
src/utils.rs Outdated
@@ -193,6 +193,9 @@ mod tests {
fs::remove_file(wallet_path).unwrap();
}
}

/// Create a wallet file with optional wallet content.
Copy link
Member

Choose a reason for hiding this comment

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

I guess I meant, what is content? I can see that it's optional, but I don't understand what content is made up of.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Content could be anything, just populates file with data. Doesn't have to be specific format.
P.s. Feel free to suggest a description (or improvement)

Copy link
Member

Choose a reason for hiding this comment

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

How about something like:

/// Represents the possible serialized states of a wallet.
/// Used primarily for simulating wallet creation and serialization processes.
enum WalletSerializedState {
    Empty,
    WithData(String),
}

/// Simulates the serialization of a wallet to a file, optionally including dummy data.
/// Primarily used to test if checks for wallet file existence are functioning correctly.
fn serialize_wallet_to_file(wallet_path: &Path, state: WalletSerializedState) {
    // Create the wallet file if it does not exist.
    if !wallet_path.exists() {
        fs::File::create(wallet_path).unwrap();
    }

    // Write content to the wallet file based on the specified state.
    if let WalletSerializedState::WithData(data) = state {
        fs::write(wallet_path, data).unwrap();
    }
}

Leaving this as a suggestion only maybe Josh has something else in mind

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks; updated: #220

Copy link
Member

Choose a reason for hiding this comment

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

Thanks Kaya, that's perfect.

alfiedotwtf
alfiedotwtf previously approved these changes Oct 29, 2024
@zees-dev zees-dev merged commit e0b0d8c into master Oct 30, 2024
12 checks passed
@zees-dev zees-dev deleted the fix/minor-wallet-issues branch October 30, 2024 00:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working testing Everything to do with testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants