feat: adding nft-fee-transaction example#34
Conversation
77fbd4d to
a05a211
Compare
AlexBrandes
left a comment
There was a problem hiding this comment.
Blocking until FeeTransaction features roll out to IntNet.
ryle-ai
left a comment
There was a problem hiding this comment.
@marcus-girolneto this has been open since October 2024 -- are you still working on it? If not, it might be worth closing to keep the PR list tidy. Happy to re-review if you pick it back up.
Changes: - Update sbt from 1.8.0 to 1.9.8 - Update Scala from 2.13.10 to 2.13.18 (match tessellation) - Use tessellation-sdk instead of individual modules (node-shared, currency-l0, currency-l1) - Update kind-projector from 0.13.2 to 0.13.4 - Update semanticdb-scalac from 4.7.1 to 4.14.2 - Fix deprecated URL constructor for Java 21 (use URI.create().toURL) - Add OSGI-INF/MANIFEST.MF assembly merge strategy Tested with tessellation/develop branch using Euclid SDK. All metagraph JARs build successfully.
Build Fixes for tessellation/developI tested this example with 1. Update sbt version (
|
|
expected to now be compatible with upcoming tessellation v4 release |
ryle-ai
left a comment
There was a problem hiding this comment.
Review: nft-fee-transaction example
This adds a new example demonstrating the FeeTransaction feature on top of the existing NFT metagraph. The structure follows the existing patterns well, and the build config has already incorporated the fixes from ottobot-ai's comment (SDK-only dependency, updated compiler plugins, OSGI merge strategy, Java 21 URI fix). A few issues need to be addressed before merging.
1. Placeholder tessellation version will break builds
examples/nft-fee-transaction/project/Dependencies.scala, line 6:
val tessellation = "99.99.99" //TODO: Update with the official version with FeeTransactionsThis is a non-existent version. Anyone cloning this example will get an immediate resolution failure. Per scasplte2's comment, this should now be compatible with the upcoming tessellation v4 release. The version needs to be set to an actual published artifact version before merge, or at minimum match what the other examples in this repo use if the FeeTransaction APIs have landed in an RC.
2. validateFee rejects all transactions without fees, contradicting the README
Both data_l1/Main.scala (around line 134) and l0/Main.scala (around line 130) implement validateFee with this pattern:
maybeFeeTransaction match {
case Some(feeTransaction) =>
dataUpdate.value match {
case _: MintCollection =>
if (feeTransaction.value.amount.value.value < 10000)
NotEnoughFee.invalidNec[Unit].pure[IO]
else
().validNec[DataApplicationValidationError].pure[IO]
case _ =>
().validNec[DataApplicationValidationError].pure[IO]
}
case None =>
MissingFeeTransaction.invalidNec[Unit].pure[IO] // <-- rejects ALL types
}The case None branch unconditionally returns MissingFeeTransaction for every update type. But the README explicitly states:
"For other updates, such as MintNFT, TransferNFT, and TransferCollection, fees are optional, so the transaction may be accepted without a fee transaction."
Either the README is wrong and all transactions require fees, or the case None branch should pattern-match on the data update type and only reject MintCollection when no fee is provided. Since this is example code that developers will copy, the inconsistency between documentation and implementation is particularly problematic.
3. Unhandled NumberFormatException in custom route
modules/l0/.../custom_routes/CustomRoutes.scala, line 67:
case GET -> Root / "collections" / collectionId / "nfts" / nftId => getCollectionNFTById(collectionId, nftId.toLong)nftId.toLong will throw an unhandled NumberFormatException if the path segment is not a valid number (e.g., /collections/abc/nfts/notanumber). This will surface as a 500 to the caller. Consider using Try(nftId.toLong).toOption with a NotFound() or BadRequest() fallback, or an http4s LongVar extractor.
4. Copy-paste naming artifact
modules/shared_data/.../calculated_state/CalculatedStateService.scala, line 36:
val currentVoteCalculatedState = currentState.stateThis variable is named currentVoteCalculatedState -- likely a leftover from the voting-poll example. Should be something like currentNFTCalculatedState or just currentCalculatedState for clarity.
ryle-ai
left a comment
There was a problem hiding this comment.
validateFee doesn't enforce the minimums that estimateFee advertises
estimateFee returns specific per-type minimums:
- MintCollection: 10,000
- MintNFT: 110,000
- TransferCollection: 120,000
- TransferNFT: 130,000
But validateFee only checks the minimum for MintCollection. The case _ => branch accepts any fee amount (even 1 datum) for the other three types. The tessellation framework does not auto-enforce estimated fees -- validateFee is the only place where per-type minimums are checked, and this example skips three of the four types.
Since this is example code that developers will copy as the canonical pattern for fee transactions, it should demonstrate correct enforcement. Either add minimum checks for all four types in validateFee, or change estimateFee to return EstimatedFee.Zero for the types where any amount is acceptable.
| NotEnoughFee.invalidNec[Unit].pure[IO] | ||
| else | ||
| ().validNec[DataApplicationValidationError].pure[IO] | ||
| case _ => |
There was a problem hiding this comment.
This wildcard accepts any fee amount for MintNFT, TransferCollection, and TransferNFT -- but estimateFee above (lines 122-125) returns 110000, 120000, and 130000 respectively for those types. The framework does not compare fee amounts against estimateFee results; that enforcement is entirely up to validateFee. A user could pay 1 datum for a TransferNFT and it would pass.
Either add per-type minimum checks here to match estimateFee, or change estimateFee to return EstimatedFee.Zero for types where any amount is fine.
| NotEnoughFee.invalidNec[Unit].pure[IO] | ||
| else | ||
| ().validNec[DataApplicationValidationError].pure[IO] | ||
| case _ => |
There was a problem hiding this comment.
Same issue as data_l1 -- this wildcard accepts any fee amount for non-MintCollection types, while estimateFee on the L1 side advertises specific minimums for each type.
Changes
Notes