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

eth: add signing function parameter to specify receive address case #90

Merged
merged 1 commit into from
Oct 31, 2024

Conversation

Tomasvrba
Copy link
Contributor

No description provided.

src/eth.rs Outdated Show resolved Hide resolved
/// Identifies the case of the recipient address given as hexadecimal string.
/// This function exists as a convenience to potentially help clients to determine the case of the
/// recipient address.
pub fn eth_identify_case(recipient_address: &str) -> pb::EthAddressCase {
Copy link
Contributor

Choose a reason for hiding this comment

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

ChatGPT suggests this improvement to not allocate a String:

pub fn eth_identify_case(recipient_address: &str) -> pb::EthAddressCase {
    if recipient_address.chars().all(|c| c.is_ascii_uppercase()) {
        pb::EthAddressCase::Upper
    } else if recipient_address.chars().all(|c| c.is_ascii_lowercase()) {
        pb::EthAddressCase::Lower
    } else {
        pb::EthAddressCase::Mixed
    }
}

Please also add a test_eth_identify_case() unit test in the mod tests { block at the bottom of the file. ChatGPT can probably make this quickly too 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed, but had to add !c.is_ascii_alphabetic() since the original one doesn't work with numbers

    if recipient_address
        .chars()
        .all(|c| !c.is_ascii_alphabetic() || c.is_ascii_uppercase())
    {
        pb::EthAddressCase::Upper
    } else if recipient_address
        .chars()
        .all(|c| !c.is_ascii_alphabetic() || c.is_ascii_lowercase())
    {
        pb::EthAddressCase::Lower
    } else {
        pb::EthAddressCase::Mixed
    }

src/wasm/types.rs Outdated Show resolved Hide resolved
@benma
Copy link
Contributor

benma commented Oct 10, 2024

Please also add a CHANGELOG in CHANGELOG-npm.md under 0.7.0 and in CHANGELOG-rust.md under 0.6.0

And also please configure your editor to run rustfmt on save - CI is complaining about unformatted code.

@Tomasvrba
Copy link
Contributor Author

Tomasvrba commented Oct 13, 2024

squash and merge when ready

edit: strange, looks like CI fails because of the missing optional argument in the examples/eth.rs want me to add it there? Any idea though why it fails when it's supposed to be optional?

@benma
Copy link
Contributor

benma commented Oct 28, 2024

squash and merge when ready

edit: strange, looks like CI fails because of the missing optional argument in the examples/eth.rs want me to add it there? Any idea though why it fails when it's supposed to be optional?

@Tomasvrba there are no optional args. Option<...> means the argument type is Option<...>, not that it can be omitted. You can provide Some(...) or None as the value 😄 Please do that, CI needs to pass.

@Tomasvrba Tomasvrba force-pushed the eth-address-case branch 2 times, most recently from 09ad8ba to 86eaa00 Compare October 29, 2024 09:52
@Tomasvrba
Copy link
Contributor Author

Please also add a CHANGELOG in CHANGELOG-npm.md under 0.7.0 and in CHANGELOG-rust.md under 0.6.0

And also please configure your editor to run rustfmt on save - CI is complaining about unformatted code.

I may be a rust noob, but this is bad naming and I will die on this hill, Option should be optional :P

@Tomasvrba Tomasvrba requested a review from benma October 29, 2024 09:56
CHANGELOG-npm.md Outdated
@@ -6,6 +6,7 @@
- btc: handle error when an input's previous transaction is required but missing
- btc: add support for regtest
- btc: add support for Taproot wallet policies
- eth: add method to help clients identify address case (upper/lower/mixed)
Copy link
Contributor

Choose a reason for hiding this comment

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

it's not just that, it also allows clients to specify the address case in the first place when signing a tx.

Copy link
Contributor

@benma benma left a comment

Choose a reason for hiding this comment

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

Thanks! Please squash, then I'll merge.

@benma benma merged commit 5f22d90 into BitBoxSwiss:master Oct 31, 2024
3 checks passed
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.

2 participants