tgerboui / nona-lib

TypeScript library to simplify interactions with the Nano currency node.
MIT License
6 stars 1 forks source link

Introduce NanoAddress type #2

Closed luxbe closed 2 months ago

luxbe commented 3 months ago

As groundwork to work on the Nano Name Service integration, I first introduced a basic NanoAddress type which represents valid Nano address in the form of nano_.+. I replaced the references to addresses from string to NanoAddress which affected more files than I initially expected.

TODO:

In the next step I plan to add a NanoUsername type to resolve actual usernames - however this is not part of this PR.

Let me know what you think of the changes and also how I should proceed with the TODOs!

Closes #1

luxbe commented 3 months ago

Another possible improvement would be to turn NanoAddress into its own class with a constructor, that could check the input string for validity

tgerboui commented 3 months ago

The changes seems goods to me for now, you can continue on this path.

Good catch about the from in the documentation, don't worry about this I will fix this in another PR, this is not the scope of this one. Otherwise you TODO is appropriate for this feature.

I'm not sure about the NanoAddress class, I think it will complicate the code and confused the user. Let keep it simple. But if you have a strong argument for using it let me know.

codecov-commenter commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 100.00%. Comparing base (950505d) to head (b7bd5c6).

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #2 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 25 29 +4 Lines 402 435 +33 Branches 66 76 +10 ========================================= + Hits 402 435 +33 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

luxbe commented 2 months ago

Should I also update the version to 1.3.0?

tgerboui commented 2 months ago

Should I also update the version to 1.3.0?

No don't worry, I will create a PR for this I think.

I'm not sure about the versioning protocol for now.

tgerboui commented 2 months ago

While reviewing, I saw that the change method of the wallet is not in the documentation of the README, I will create an issue.