- Notifications
You must be signed in to change notification settings - Fork26
chore/bump-version-1.0.0-SNAPSHOT#474
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
- Clarify the process for importing the BOM in both Maven and Gradle.- Emphasize omitting per-module versions for better dependency management.BREAKING CHANGE: users must update their `pom.xml` and `build.gradle` files.Impact: existing configurations may break if not updated.Migration: follow the new steps outlined in the migration document.
- Changed specific version numbers to placeholders for better guidance.- Clarified instructions on managing transitive dependencies with BOM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Pull Request Overview
This PR bumps the project version to 1.0.0-SNAPSHOT and establishes comprehensive release infrastructure for the 1.0 milestone. It removes deprecated APIs scheduled for deletion, adds CI/CD workflows, and completes documentation for stable release processes.
Key Changes
- Version bump from 0.6.5-SNAPSHOT to 1.0.0-SNAPSHOT across all modules
- Removal of deprecated APIs (Constants.Kind facade, legacy encoder singleton, deprecated constructors)
- Addition of CI/CD workflows with matrix builds and Docker-based integration tests
- Complete release automation script with bump/verify/tag/publish/next-snapshot commands
Reviewed Changes
Copilot reviewed 63 out of 63 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| pom.xml and module poms | Version bumped to 1.0.0-SNAPSHOT |
| scripts/release.sh | New release automation script with comprehensive workflow commands |
| nostr-java-base/src/main/java/nostr/base/Encoder.java | Removed deprecated ENCODER_MAPPER_BLACKBIRD constant |
| nostr-java-api/src/main/java/nostr/config/Constants.java | Removed deprecated Constants.Kind facade |
| nostr-java-api/src/main/java/nostr/api/NIP01.java | Removed deprecated createTextNoteEvent(Identity, String) overload |
| .github/workflows/ci.yml | New CI workflow with Java 21/17 matrix and separate Docker IT job |
| .github/workflows/release.yml | New release workflow for automated publishing to Maven Central |
| docs/ | Comprehensive documentation updates for BOM usage and release processes |
| $no_docker&& mvn_args=(-q -DnoDocker=true -P release deploy) | ||
| $skip_tests&& mvn_args=(-q -DskipTests -P release deploy) | ||
| if$no_docker&&$skip_tests;then mvn_args=(-q -DskipTests -DnoDocker=true -P release deploy);fi |
CopilotAIOct 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The Maven argument construction logic is duplicated and hard to maintain. Consider building the array incrementally to avoid repetition and make it easier to extend with new flags.
| $no_docker&& mvn_args=(-q -DnoDocker=true -P release deploy) | |
| $skip_tests&& mvn_args=(-q -DskipTests -P release deploy) | |
| if$no_docker&&$skip_tests;then mvn_args=(-q -DskipTests -DnoDocker=true -P release deploy);fi | |
| $no_docker&& mvn_args+=(-DnoDocker=true) | |
| $skip_tests&& mvn_args+=(-DskipTests) |
| # Look up an existing project with the desired title. | ||
| project_number=$(gh project list --owner"${repo_owner}" --format json| | ||
| jq -r --arg title"${project_title}"'map(select(.title == $title)) | first | .number // empty') | ||
| jq -r --arg title"${project_title}"'[.. | objects | select(has("title")) |select(.title == $title)] | first? | .number // empty') |
CopilotAIOct 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The jq expression is complex and hard to read. The recursive descent with.. followed by filtering could have performance implications on large project lists. Consider using a more direct approach likemap(select(.title == $title)) | first | .number // empty if the structure is predictable.
| jq -r --arg title"${project_title}"'[.. | objects | select(has("title")) |select(.title == $title)] | first? | .number // empty') | |
| jq -r --arg title"${project_title}"'map(select(.title == $title)) | first | .number // empty') |
| } | ||
| Stringlower =bolt11.toLowerCase(Locale.ROOT); | ||
| intsep =lower.indexOf('1'); | ||
| intsep =lower.lastIndexOf('1'); |
CopilotAIOct 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
UsinglastIndexOf instead ofindexOf for finding the separator in BOLT11 invoices could be incorrect. BOLT11 format specifies a single '1' separator between HRP and data, soindexOf should be used to find the first occurrence.
| intsep =lower.lastIndexOf('1'); | |
| intsep =lower.indexOf('1'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@codex address this comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Summary
- Updated BOLT11 parsing to locate the first HRP separator, ensuring invoices are split at the mandated single separator.nostr-java-api/src/main/java/nostr/api/nip57/Bolt11Util.javaL20-L28
Testing
- ❌
mvn -q verify(fails: Non-resolvable import POM xyz.tcheeric:nostr-java-bom:pom:1.1.1)
| // Parses BTC with no unit. Example: 1 BTC → 100,000,000,000 msat. | ||
| voidparseWholeBtcNoUnit() { | ||
| longmsat =Bolt11Util.parseMsat("lnbc1psome"); | ||
| longmsat =Bolt11Util.parseMsat("lnbc11psome"); |
CopilotAIOct 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The test invoicelnbc11psome appears to have an invalid format. BOLT11 invoices should have the amount before the unit, so this should belnbc1p1some (1 BTC) rather thanlnbc11psome which suggests 11 pico-BTC.
| longmsat =Bolt11Util.parseMsat("lnbc11psome"); | |
| longmsat =Bolt11Util.parseMsat("lnbc1p1some"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@codex address this comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Summary
- Updated the whole-BTC parsing fixture in
Bolt11UtilTestto use a valid BOLT11 invoice string that encodes the amount before the unit.nostr-java-api/src/test/java/nostr/api/unit/Bolt11UtilTest.javaL58-L61
Testing
⚠️ mvn -q verify(fails: missing dependency xyz.tcheeric:nostr-java-bom:1.1.1 in Maven Central)
| * Manages the lifecycle of {@link WebSocketClientHandler} instances keyed by relay name. | ||
| */ | ||
| publicfinalclassNostrRelayRegistry { | ||
| publicclassNostrRelayRegistry { |
CopilotAIOct 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Changing the class fromfinal to non-final may have been unintentional and could impact the design. If this class was intentionally made final for safety reasons, removing final could allow unintended inheritance.
| publicclassNostrRelayRegistry { | |
| publicfinalclassNostrRelayRegistry { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
💡 Codex Review
nostr-java/nostr-java-api/src/main/java/nostr/api/NIP61.java
Lines 92 to 118 in85985de
| /** | |
| * Create a Nutzap event (kind 7374). | |
| * | |
| * @param proofs list of Cashu proofs | |
| * @param url the mint URL | |
| * @param nutzappedEventTag optional event being zapped (e-tag) | |
| * @param recipient the recipient public key (p-tag) | |
| * @param content optional human-readable content | |
| * @return this instance for chaining | |
| */ | |
| publicNIP61createNutzapEvent( | |
| List<CashuProof>proofs, | |
| @NonNullURLurl, | |
| EventTagnutzappedEventTag, | |
| @NonNullPublicKeyrecipient, | |
| @NonNullStringcontent) { | |
| GenericEventgenericEvent = | |
| newGenericEventFactory(getSender(),Kind.NUTZAP.getValue(),content).create(); | |
| proofs.forEach(proof ->genericEvent.addTag(NIP61.createProofTag(proof))); | |
| if (nutzappedEventTag !=null) { | |
| genericEvent.addTag(nutzappedEventTag); | |
| } | |
| genericEvent.addTag(NIP61.createUrlTag(url.toString())); | |
| genericEvent.addTag(NIP01.createPubKeyTag(recipient)); |
The refactoredcreateNutzapEvent builder now only adds proofs, URL, and recipient tags. The previous version also attachedamount andunit tags viaNIP60.createAmountTag/createUnitTag, which are required for a valid NIP‑61 nutzap event. Callers ofcreateNutzapEvent(NutZap, …) will now emit events missing these tags unless they remember to add them manually (the unit test now does so after calling the method). This silently produces malformed events in existing code that relied on the builder to produce a complete zap payload.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting
| -name:Setup Java 21 with Maven Central credentials and GPG | ||
| uses:actions/setup-java@v4 | ||
| with: | ||
| distribution:temurin | ||
| java-version:'21' | ||
| cache:maven | ||
| server-id:central | ||
| server-username:CENTRAL_USERNAME | ||
| server-password:CENTRAL_PASSWORD |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Use Maven Central secrets when configuring setup-java
The release workflow configuresactions/setup-java with literal stringsCENTRAL_USERNAME/CENTRAL_PASSWORD instead of the secrets that are exported later for the publish step. This causes the action to generate asettings.xml containing those literal values, so the subsequentmvn … deploy will authenticate with the wrong credentials and the release job will fail with 401. Pass${{ secrets.CENTRAL_USERNAME }} and${{ secrets.CENTRAL_PASSWORD }} toserver-username/server-password so the credentials are written correctly.
Useful? React with 👍 / 👎.
…-bolt11utiltesttest: fix bolt11 whole btc fixture
- Update the logic to find the HRP separator in BOLT11 invoices by using lastIndexOf instead of indexOf. This ensures proper parsing of invoices with multiple '1' characters.- Adjust test case to reflect the change in expected input format.
- Ensure that the uuid from the identifier tag is appended only if it is not null.- This prevents potential null pointer exceptions and improves stability.
- Added functionality to check for existing tasks before creating new ones.- Implemented assignment of tasks to the current GitHub user if available.- Improved logging for task creation and updates to enhance user feedback.
de0f48a intodevelopUh oh!
There was an error while loading.Please reload this page.
Summary
Bump project version to 1.0.0-SNAPSHOT and align docs/automation for the 1.0 track.
This prepares the repository for final 1.0 changes and release workflows.
Related issue: #
What changed?
BREAKING
No functional API changes in this PR. Upcoming 1.0.0 will remove deprecated APIs as already documented in MIGRATION.md.
Review focus
Checklist