HIP-18 enhancement #2419
HIP-18 enhancement #2419
Conversation
- add sql script to add missing token-treasury association Signed-off-by: Xin Li <[email protected]>
…andling Signed-off-by: Xin Li <[email protected]>
Signed-off-by: Xin Li <[email protected]>
Signed-off-by: Xin Li <[email protected]>
… the same way for treasury and auto enabled fee collectors Signed-off-by: Xin Li <[email protected]>
…ssociated Signed-off-by: Xin Li <[email protected]>
Signed-off-by: Xin Li <[email protected]>
Signed-off-by: Xin Li <[email protected]>
Signed-off-by: Xin Li <[email protected]>
Signed-off-by: Xin Li <[email protected]>
Signed-off-by: Xin Li <[email protected]>
Signed-off-by: Xin Li <[email protected]>
Signed-off-by: Xin Li <[email protected]>
Signed-off-by: Xin Li <[email protected]>
Signed-off-by: Xin Li <[email protected]>
Signed-off-by: Xin Li <[email protected]>
Signed-off-by: Xin Li <[email protected]>
xin-hedera
added
enhancement
Codecov Report
@@ Coverage Diff @@
## main #2419 +/- ##
============================================
+ Coverage 84.42% 84.49% +0.07%
- Complexity 2335 2346 +11
============================================
Files 440 441 +1
Lines 12018 12073 +55
Branches 1024 1029 +5
============================================
+ Hits 10146 10201 +55
Misses 1554 1554
Partials 318 318
Continue to review full report at Codecov.
|
| @@ -61,7 +61,7 @@ | |||
| "custom_fees": { | |||
| "created_timestamp": "1234567890.000000002", | |||
| "fixed_fees": [], | |||
| "fractional_fees": [] | |||
| "royalty_fees": [] | |||
There was a problem hiding this comment.
this is indeed a bug: nft token type won't have fractional_fees
Signed-off-by: Xin Li <[email protected]>
…tests Signed-off-by: Xin Li <[email protected]>
| * @param tokenId the attached token id | ||
| * @return whether the fee is paid in the attached token | ||
| */ | ||
| private boolean parseFixedFee(CustomFee customFee, FixedFee fixedFee, EntityId tokenId) { |
There was a problem hiding this comment.
function is doing 2 things.
Better to simplify by removing return logic and moving check to it's own method in the CustomFee class
private boolean isChargedInToken(EntityId tokenId) {
return !EntityId.isEmpty(tokenId) && denominatingTokenId.equals(tokenId);
}There was a problem hiding this comment.
made the change to encapsulate the logic: the function parseFixedFee does the parsing and so it surely knows whether the fixed fee's denominating token is the attached token.
For the other two types of fees the answer to the question 'do you charge in the attached token' is rather obvious, so doesn't need to follow the pattern.
There was a problem hiding this comment.
I see the change here.
Not fully what I was suggesting as I was suggesting move the logic to the CustomFee domain class so that this method returned void. Then in the FIXED FEE case above you'd update the call flow.
Highlighted the example below.
parseFixedFee(customFee, protoCustomFee.getFixedFee(), tokenId);
chargedInAttachedToken = customFee.isChargedInToken(tokenId);In fact if we passed along the protoCustomFee.getFeeCase() to the CustomFee domain it would further simplify and centralize the chargedInAttachedToken logic.
No need to change it here though
.../src/main/java/com/hedera/mirror/importer/parser/record/entity/EntityRecordItemListener.java
Outdated
Show resolved
Hide resolved
| if (token.type === 'NON_FUNGIBLE_UNIQUE') { | ||
| token.decimals = 0; | ||
| token.initial_supply = 0; | ||
| } |
There was a problem hiding this comment.
NFTs must have decimals = 0 and initial_supply = 0 according to HAPI spec
Signed-off-by: Xin Li <[email protected]>
docs/design/custom-fees.md
Outdated
| amount bigint not null, | ||
| collector_account_id bigint not null, | ||
| consensus_timestamp bigint not null, | ||
| effective_payer_account_id bigint, |
There was a problem hiding this comment.
This duplicates all info for each effective payer account. We should normalize this out into another table assessed_custom_fee_payer to reduce the duplication.
There was a problem hiding this comment.
that'll actually create more duplication since that table will be the same as the new assessed_custom_fee table: only (amount, collector_account_id, consensus_timestamp, token_id) uniquely identifies one assessed custom fee in a transaction paid by a list of payers to the same collector in the same denominating token (or hbar)...
There was a problem hiding this comment.
postgresql supports arrays, we can alternatively have such a single column effective_payer_account_ids
There was a problem hiding this comment.
If we don't use the array, the idea would be to add a generated ID (UUID preferrably) as primary key to assessed_custom_fee and have a foreign key. Then the other table only needs foreign ID and payer account. We kind of need a primary key anyway as there's none now and distributed sqls need one.
There was a problem hiding this comment.
changed to bigint[] array, had to workaround the entityid to long conversion though.
.../src/main/java/com/hedera/mirror/importer/parser/record/entity/EntityRecordItemListener.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Xin Li <[email protected]>
| @EmbeddedId | ||
| @JsonUnwrapped | ||
| private Id id; | ||
|
|
||
| private long amount; | ||
|
|
||
| @Type(type = "com.vladmihalcea.hibernate.type.array.ListArrayType") | ||
| @JsonSerialize(using = LongListToStringSerializer.class) | ||
| private List<Long> effectivePayerAccountIds = Collections.emptyList(); |
There was a problem hiding this comment.
can't use AccountIdConverter with hibernate-types ListArrayType
| @Override | ||
| public void serialize(List<Long> longs, JsonGenerator gen, SerializerProvider serializers) throws IOException { | ||
| if (longs != null) { | ||
| gen.writeString("{" + StringUtils.join(longs, ",") + "}"); |
There was a problem hiding this comment.
the encoding to insert array to postgresql
...-importer/src/main/java/com/hedera/mirror/importer/converter/LongListToStringSerializer.java
Show resolved
Hide resolved
hedera-mirror-importer/src/main/java/com/hedera/mirror/importer/domain/AssessedCustomFee.java
Outdated
Show resolved
Hide resolved
.../src/main/java/com/hedera/mirror/importer/parser/record/entity/EntityRecordItemListener.java
Show resolved
Hide resolved
hedera-mirror-importer/src/main/resources/db/migration/v2/V2.0.0__time_scale_init.sql
Show resolved
Hide resolved
Signed-off-by: Xin Li <[email protected]>
Signed-off-by: Xin Li <[email protected]>
|
Kudos, SonarCloud Quality Gate passed!
|
| * @param tokenId the attached token id | ||
| * @return whether the fee is paid in the attached token | ||
| */ | ||
| private boolean parseFixedFee(CustomFee customFee, FixedFee fixedFee, EntityId tokenId) { |
There was a problem hiding this comment.
I see the change here.
Not fully what I was suggesting as I was suggesting move the logic to the CustomFee domain class so that this method returned void. Then in the FIXED FEE case above you'd update the call flow.
Highlighted the example below.
parseFixedFee(customFee, protoCustomFee.getFixedFee(), tokenId);
chargedInAttachedToken = customFee.isChargedInToken(tokenId);In fact if we passed along the protoCustomFee.getFeeCase() to the CustomFee domain it would further simplify and centralize the chargedInAttachedToken logic.
No need to change it here though
| "description": "Token info api call for a given non-fungible token with custom fees", | ||
| "extendedDescription": [ | ||
| "The token has 3 custom fees schedules: an empty schedule at creation, a single custom fee schedule at", | ||
| "1234567899999999001, and a 4 custom fees schedule at 1234567899999999007. Without the timestamp filter, the query", |
There was a problem hiding this comment.
nit:
| "1234567899999999001, and a 4 custom fees schedule at 1234567899999999007. Without the timestamp filter, the query", | |
| "1234567899999999001, and a 5 custom fees schedule at 1234567899999999007. Without the timestamp filter, the query", |
| @@ -61,8 +69,23 @@ class CustomFeeViewModel { | |||
| return !!this.amount; | |||
| } | |||
|
|
|||
| isFixedFee() { | |||
There was a problem hiding this comment.
nit: connected to my other comment if we passed along the type these checks would be simpler.
Although it would require persistence with an additional type column in `custom_fee table
Description:
This PR adds support for several HIP-18 enhancement.
Related issue(s):
Fixes #2377
Fixes #2378
Fixes #2379
Notes for reviewer:
protobuf response code changes:
$ git diff v0.16.0 v0.17.1 src/main/proto/ResponseCode.proto diff --git a/src/main/proto/ResponseCode.proto b/src/main/proto/ResponseCode.proto index ae05c3b..2c4f980 100644 --- a/src/main/proto/ResponseCode.proto +++ b/src/main/proto/ResponseCode.proto @@ -240,7 +240,7 @@ enum ResponseCodeEnum { BATCH_SIZE_LIMIT_EXCEEDED = 228; // Repeated operations count exceeds the limit INVALID_QUERY_RANGE = 229; // The range of data to be gathered is out of the set boundaries FRACTION_DIVIDES_BY_ZERO = 230; // A custom fractional fee set a denominator of zero - INSUFFICIENT_PAYER_BALANCE_FOR_CUSTOM_FEE = 231; // The transaction payer could not afford a custom fee + INSUFFICIENT_PAYER_BALANCE_FOR_CUSTOM_FEE = 231 [deprecated = true]; // The transaction payer could not afford a custom fee CUSTOM_FEES_LIST_TOO_LONG = 232; // More than 10 custom fees were specified INVALID_CUSTOM_FEE_COLLECTOR = 233; // Any of the feeCollector accounts for customFees is invalid INVALID_TOKEN_ID_IN_CUSTOM_FEES = 234; // Any of the token Ids in customFees is invalid @@ -251,7 +251,7 @@ enum ResponseCodeEnum { CUSTOM_FEE_MUST_BE_POSITIVE = 239; // Only positive fees may be assessed at this time TOKEN_HAS_NO_FEE_SCHEDULE_KEY = 240; // Fee schedule key is not set on token CUSTOM_FEE_OUTSIDE_NUMERIC_RANGE = 241; // A fractional custom fee exceeded the range of a 64-bit signed integer - INVALID_CUSTOM_FRACTIONAL_FEES_SUM = 242; // The sum of all custom fractional fees must be strictly less than 1 + ROYALTY_FRACTION_CANNOT_EXCEED_ONE = 242; // A royalty cannot exceed the total fungible value exchanged for an NFT FRACTIONAL_FEE_MAX_AMOUNT_LESS_THAN_MIN_AMOUNT = 243; // Each fractional custom fee must have its maximum_amount, if specified, at least its minimum_amount CUSTOM_SCHEDULE_ALREADY_HAS_NO_FEES = 244; // A fee schedule update tried to clear the custom fees from a token whose fee schedule was already empty CUSTOM_FEE_DENOMINATION_MUST_BE_FUNGIBLE_COMMON = 245; // Only tokens of type FUNGIBLE_COMMON can be used to as fee schedule denominations @@ -268,4 +268,7 @@ enum ResponseCodeEnum { PAYER_ACCOUNT_DELETED = 256; // The payer account has been marked as deleted CUSTOM_FEE_CHARGING_EXCEEDED_MAX_RECURSION_DEPTH = 257; // The reference chain of custom fees for a transferred token exceeded the maximum length of 2 CUSTOM_FEE_CHARGING_EXCEEDED_MAX_ACCOUNT_AMOUNTS = 258; // More than 20 balance adjustments were to satisfy a CryptoTransfer and its implied custom fee payments + INSUFFICIENT_SENDER_ACCOUNT_BALANCE_FOR_CUSTOM_FEE = 259; // The sender account in the token transfer transaction could not afford a custom fee + SERIAL_NUMBER_LIMIT_REACHED = 260; // Currently no more than 4,294,967,295 NFTs may be minted for a given unique token type + CUSTOM_ROYALTY_FEE_ONLY_ALLOWED_FOR_NON_FUNGIBLE_UNIQUE = 261; // Only tokens of type NON_FUNGIBLE_UNIQUE can have royalty fees }Checklist