feat: no shard realm encoding in long zero #19349

lukelee-sl · 2025-05-27T16:04:38Z

Description:
Remove the encoding of the shard and realm in the long zero(account number alias) addresses

Related issue(s):

Fixes #19181

Documented (Code comments, README, etc.)
Tested (unit, integration, etc.)

Signed-off-by: Luke Lee <[email protected]>

…nto 19181-no-shard-realm-in-long-zero

Signed-off-by: Luke Lee <[email protected]>

…nto 19181-no-shard-realm-in-long-zero

codecov · 2025-05-27T16:58:23Z

Codecov Report

Attention: Patch coverage is 79.01786% with 47 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...ontracts/hts/transfer/ClassicTransfersDecoder.java	37.50%	25 Missing ⚠️
...racts/has/isauthorizedraw/IsAuthorizedRawCall.java	25.00%	2 Missing and 1 partial ⚠️
...ystemcontracts/hts/create/CreateCommonDecoder.java	33.33%	2 Missing ⚠️
...ystemcontracts/hts/update/UpdateCommonDecoder.java	33.33%	2 Missing ⚠️
...tracts/hts/update/address_0x16c/UpdateDecoder.java	50.00%	2 Missing ⚠️
...ntract/impl/exec/v038/Version038AddressChecks.java	0.00%	0 Missing and 2 partials ⚠️
...p/service/contract/impl/utils/ConversionUtils.java	90.90%	1 Missing and 1 partial ⚠️
.../hedera/node/app/throttle/ThrottleAccumulator.java	0.00%	0 Missing and 1 partial ⚠️
.../impl/exec/systemcontracts/has/HasCallAttempt.java	0.00%	0 Missing and 1 partial ⚠️
.../impl/exec/systemcontracts/hss/HssCallAttempt.java	0.00%	0 Missing and 1 partial ⚠️
... and 6 more

@@             Coverage Diff              @@
##               main   #19349      +/-   ##
============================================
- Coverage     69.36%   69.35%   -0.02%     
+ Complexity    23305    23301       -4     
============================================
  Files          2646     2646              
  Lines        100908   100896      -12     
  Branches      10441    10438       -3     
============================================
- Hits          69998    69979      -19     
- Misses        26931    26937       +6     
- Partials       3979     3980       +1

Files with missing lines	Coverage Δ	Complexity Δ
...va/com/hedera/node/app/hapi/utils/CommonUtils.java	`72.72% <ø> (-1.19%)`	`20.00 <0.00> (ø)`
...de/app/service/contract/impl/exec/FrameRunner.java	`96.61% <100.00%> (ø)`	`16.00 <2.00> (ø)`
...ract/impl/exec/operations/CustomCallOperation.java	`76.19% <100.00%> (ø)`	`13.00 <4.00> (ø)`
...ts/has/getevmaddressalias/EvmAddressAliasCall.java	`100.00% <100.00%> (ø)`	`6.00 <0.00> (ø)`
...deraaccountnumalias/HederaAccountNumAliasCall.java	`80.00% <100.00%> (-0.96%)`	`4.00 <0.00> (ø)`
...emcontracts/has/isvalidalias/IsValidAliasCall.java	`100.00% <100.00%> (ø)`	`8.00 <4.00> (ø)`
.../impl/exec/systemcontracts/hts/HtsCallAttempt.java	`79.31% <100.00%> (ø)`	`13.00 <0.00> (ø)`
...temcontracts/hts/airdrops/TokenAirdropDecoder.java	`100.00% <100.00%> (ø)`	`16.00 <1.00> (ø)`
...ontracts/hts/associations/AssociationsDecoder.java	`100.00% <100.00%> (ø)`	`15.00 <4.00> (ø)`
...systemcontracts/hts/create/ClassicCreatesCall.java	`59.30% <100.00%> (-0.47%)`	`10.00 <0.00> (ø)`
... and 38 more

... and 3 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

codacy-production · 2025-05-27T16:58:26Z

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
✅ -0.01% (target: -1.00%)	✅ 83.93%

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (`60f8562`)	100813	73930	73.33%
Head commit (`1cfa575`)	100801 (-12)	73912 (-18)	73.32% (-0.01%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#19349)	224	188	83.93%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings Change summary preferences

Signed-off-by: Luke Lee <[email protected]>

…nto 19181-no-shard-realm-in-long-zero

Signed-off-by: Luke Lee <[email protected]>

…nto 19181-no-shard-realm-in-long-zero

Signed-off-by: Luke Lee <[email protected]>

…encoded Signed-off-by: Luke Lee <[email protected]>

Signed-off-by: Luke Lee <[email protected]>

…nto 19181-no-shard-realm-in-long-zero

Signed-off-by: Luke Lee <[email protected]>

…nto 19181-no-shard-realm-in-long-zero

gkozyryatskyy · 2025-06-04T17:06:13Z

...ra/node/app/service/contract/impl/exec/systemcontracts/hts/airdrops/TokenAirdropDecoder.java

    @Inject
    public TokenAirdropDecoder() {
        // Dagger2
    }

    public TransactionBody decodeAirdrop(@NonNull final HtsCallAttempt attempt) {
+        this.attempt = requireNonNull(attempt);


put attempt.nativeOperations().entityIdFactory() to bodyForAirdrop function instead?

mhess-swl

A couple of general things:

I'm not fond of passing entityIdFactory everywhere in order to resolve the shard/realm, at least not by that name. Its meaning is too close to the incremental entity ID counter stored in state.
Not sure why we're not also stripping the 0.0 shard/realm out of long zero encodings, but there very well could be nuance here I'm not aware of.

Aside from these, the changes overall make sense. This is a lot of slog work–thanks @lukelee-sl!!

mhess-swl · 2025-06-04T21:53:16Z

...ents/src/main/java/com/hedera/services/bdd/suites/contract/validation/EvmValidationTest.java

-                        (spec) -> toAddressStringWithShardAndRealm((int) spec.shard(), spec.realm(), "1117d0");
+                        (spec) -> toAddressStringWithShardAndRealm(0, 0, "1117d0");


If the purpose of this PR is to remove shard/realm encoding in the address string, why wouldn't shard/realm be removed entirely? Should toAddressStringWithShardAndRealm() exist at all anymore?

mhess-swl · 2025-06-04T21:55:13Z

.../src/test/java/com/hedera/node/app/service/contract/impl/test/utils/ConversionUtilsTest.java

+        final long shard = 0L;
+        final long realm = 0L;


Seems like these shouldn't be needed at all anymore, right?

mhess-swl · 2025-06-04T21:57:28Z

.../src/test/java/com/hedera/node/app/service/contract/impl/test/utils/ConversionUtilsTest.java

+        assertEquals(0, tokenId.shardNum());
+        assertEquals(0, tokenId.realmNum());


Again I'm confused–if we're removing shard/realm from the address string, how can we assert anything about a token ID's shard/realm?

mhess-swl · 2025-06-04T22:18:25Z

...vice-impl/src/main/java/com/hedera/node/app/service/contract/impl/utils/ConversionUtils.java

+        final var tokenNum = numberOfLongZero(explicit);
+        return tokenNum == 0 ? TokenID.DEFAULT : entityIdFactory.newTokenId(tokenNum);


⚠️ Subjective opinion ⚠️

I had to look at EntityIdFactory to figure out why it's being used here (and many other places). Imo it's much more straightforward for this static method (and others like it) to explicitly require a shard and realm parameter to make clear what the code itself is doing. Two things made this confusing on first read: first, using entityIdFactory simply because it already has the configured shard/realm embedded inside; and second, the newTokenId method makes it sound like a new entity ID is being generated, not just instantiated.

I do understand it's easier to pass one object than it is to pass two, but it has thrown me for a loop the past hal hour or so to understand why this object is suddenly necessary in a bunch of places.

P.S.:

* <p><b>IMPORTANT:</b> Mono-service ignores the shard and realm, c.f. De * codingFacade#convertAddressBytesToTokenID(), so we continue to do that here; might * want to revisit this later

This doc seems to be incorrect

mhess-swl · 2025-06-04T22:25:08Z

.../app/service/contract/impl/exec/systemcontracts/hts/transfer/ClassicTransfersTranslator.java

-            return decoder.decodeCryptoTransfer(attempt.inputBytes(), attempt.addressIdConverter());
+            return decoder.decodeCryptoTransfer(attempt.inputBytes(), attempt);


Out of curiosity, why are both the entityIdFactory and the addressIdConverter necessary inside of decodeCryptoTransfer(...)? From a purely conceptual standpoint, it seems like addressIdConverter alone could handle the translation (including shard/realm). Maybe its job is more specific than this, though?

mhess-swl · 2025-06-04T22:33:32Z

...ntract/impl/exec/systemcontracts/hts/updatetokencustomfees/UpdateTokenCustomFeesDecoder.java

+        final var tokenId = ConversionUtils.asTokenId(attempt.nativeOperations().entityIdFactory(), address);
+        return useHbarsForPayment
+                        || tokenId.equals(
+                                attempt.nativeOperations().entityIdFactory().newTokenId(0))


Seems like unnecessary work to convert address to a fully-qualified tokenId, convert 0 to a token ID, and then compare them. What about something similar to this?

Suggested change

final var tokenId = ConversionUtils.asTokenId(attempt.nativeOperations().entityIdFactory(), address);

return useHbarsForPayment

|| tokenId.equals(

attempt.nativeOperations().entityIdFactory().newTokenId(0))

final var tokenNum = ConversionUtils.extractTokenNum(address);

return useHbarsForPayment || tokenNum == 0

mhess-swl · 2025-06-04T22:40:30Z

...ntract/impl/exec/systemcontracts/hts/updatetokencustomfees/UpdateTokenCustomFeesDecoder.java

+        return ConversionUtils.asTokenId(attempt.nativeOperations().entityIdFactory(), tokenAddress)
+                                .equals(TokenID.DEFAULT)
+                        && amount == 0


This change itself makes sense, but is the method's logic actually correct? It could end up comparing token 5.6.0 to token 0.0.0, which would evaluate to false, but seems like maybe the intent is actually just to compare the token numbers?

For more context, I'm not quite clear on TokenID.DEFAULT's semantic meaning when the shard/realm aren't 0.0. Is TokenID.DEFAULT the only sentinel value, i.e. for shard=realm=0 (0.0.0), or is 1.2.0 also considered a sentinel value for a node using shard=1,realm=2?

mhess-swl · 2025-06-04T22:46:08Z

...ra-node/hedera-token-service/src/main/java/com/hedera/node/app/service/token/AliasUtils.java

+     * based on the entity number, the first 12 bytes will be defined to be zero which indicates the current networks
+     * shard and realm, while the last 8 bytes represent the entity number. Because the shard and realm are zero, this
+     * prefix is all zeros, which is why it is sometimes known as the "long-zero" alias.


To check my understanding, I read here that a prefix of all zeros indicates the current network's shard/realm is 0.0...but isn't the whole point of this PR to not encode shard/realm in this alias? Seems like a contradiction to me, and one that could easily cause bugs

mhess-swl · 2025-06-04T22:47:50Z

...ode/hedera-token-service/src/test/java/com/hedera/node/app/service/token/AliasUtilsTest.java

    void extractRealmFromAddressAliasReturnsZeroForZeroRealm() {
        var alias = Bytes.wrap(HexFormat.of().parseHex(TEST_ENTITY_NUM_ALIAS));
-        assertEquals(0L, AliasUtils.extractRealmFromAddressAlias(alias));
+        assertEquals(20L, AliasUtils.extractRealmFromAddressAlias(alias, 20));
    }


Should extractRealmFromAddressAlias(...) even exist if we're removing the shard/realm encoding from the alias? At least the test name should probably change

lukelee-sl added 4 commits May 15, 2025 12:15

feat: revert encoding shard realm in long zero

49345cb

Signed-off-by: Luke Lee <[email protected]>

Merge branch 'main' of https://github.com/hashgraph/hedera-services i…

e209200

…nto 19181-no-shard-realm-in-long-zero

Merge branch 'main' of https://github.com/hashgraph/hedera-services i…

a37adc0

…nto 19181-no-shard-realm-in-long-zero

revert changes for setting shard/realm

c9e3ba1

Signed-off-by: Luke Lee <[email protected]>

lukelee-sl added this to the v0.64 milestone May 27, 2025

lukelee-sl self-assigned this May 27, 2025

lukelee-sl requested review from tinker-michaelj and a team as code owners May 27, 2025 16:04

Merge branch 'main' of https://github.com/hashgraph/hedera-services i…

44fa2db

…nto 19181-no-shard-realm-in-long-zero

fix unit tests

8c308df

Signed-off-by: Luke Lee <[email protected]>

lukelee-sl marked this pull request as draft May 27, 2025 19:02

lukelee-sl added 16 commits May 28, 2025 15:27

Merge branch 'main' of https://github.com/hashgraph/hedera-services i…

3ca6702

…nto 19181-no-shard-realm-in-long-zero

remove shard/realm from asEvmAddress

35087ec

Signed-off-by: Luke Lee <[email protected]>

remove asEvmAddress override

388b33c

Signed-off-by: Luke Lee <[email protected]>

fix unit test

4883ef7

Signed-off-by: Luke Lee <[email protected]>

remove shard/realm encoding in hapi tests

a39a90a

Signed-off-by: Luke Lee <[email protected]>

Merge branch 'main' of https://github.com/hashgraph/hedera-services i…

fd2ea11

…nto 19181-no-shard-realm-in-long-zero

update asTokenId to use the EntityIdFactory to create a new token ID

457eaf9

Signed-off-by: Luke Lee <[email protected]>

isEntityNumAlias should only support aliases with no realm and shard …

5849d60

…encoded Signed-off-by: Luke Lee <[email protected]>

spotless

db37837

Signed-off-by: Luke Lee <[email protected]>

remove unneeded dependency

5248e25

Signed-off-by: Luke Lee <[email protected]>

fix hapi-utils long zero encoding

e309b21

Signed-off-by: Luke Lee <[email protected]>

fix broken hapi tests

6d0ce4c

Signed-off-by: Luke Lee <[email protected]>

Merge branch 'main' of https://github.com/hashgraph/hedera-services i…

61ea702

…nto 19181-no-shard-realm-in-long-zero

fix breaking hapi tests

fcf1b0b

Signed-off-by: Luke Lee <[email protected]>

fix breaking hapi tests

28307f8

Signed-off-by: Luke Lee <[email protected]>

Merge branch 'main' of https://github.com/hashgraph/hedera-services i…

1cfa575

…nto 19181-no-shard-realm-in-long-zero

lukelee-sl marked this pull request as ready for review June 4, 2025 16:04

lukelee-sl requested a review from a team as a code owner June 4, 2025 16:04

gkozyryatskyy approved these changes Jun 4, 2025

View reviewed changes

mhess-swl reviewed Jun 4, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat: no shard realm encoding in long zero #19349

feat: no shard realm encoding in long zero #19349

lukelee-sl commented May 27, 2025

Uh oh!

codecov bot commented May 27, 2025 •

edited

Loading

Uh oh!

codacy-production bot commented May 27, 2025 •

edited

Loading

Uh oh!

gkozyryatskyy Jun 4, 2025

Uh oh!

mhess-swl left a comment

Uh oh!

mhess-swl Jun 4, 2025

Uh oh!

mhess-swl Jun 4, 2025

Uh oh!

mhess-swl Jun 4, 2025

Uh oh!

mhess-swl Jun 4, 2025 •

edited

Loading

Uh oh!

mhess-swl Jun 4, 2025

Uh oh!

mhess-swl Jun 4, 2025

Uh oh!

mhess-swl Jun 4, 2025

Uh oh!

mhess-swl Jun 4, 2025

Uh oh!

mhess-swl Jun 4, 2025

Uh oh!

Uh oh!

		(spec) -> toAddressStringWithShardAndRealm((int) spec.shard(), spec.realm(), "1117d0");
		(spec) -> toAddressStringWithShardAndRealm(0, 0, "1117d0");

		assertEquals(0, tokenId.shardNum());
		assertEquals(0, tokenId.realmNum());

		final var tokenNum = numberOfLongZero(explicit);
		return tokenNum == 0 ? TokenID.DEFAULT : entityIdFactory.newTokenId(tokenNum);

		return decoder.decodeCryptoTransfer(attempt.inputBytes(), attempt.addressIdConverter());
		return decoder.decodeCryptoTransfer(attempt.inputBytes(), attempt);

feat: no shard realm encoding in long zero #19349

Are you sure you want to change the base?

feat: no shard realm encoding in long zero #19349

Conversation

lukelee-sl commented May 27, 2025

Uh oh!

codecov bot commented May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

codacy-production bot commented May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage summary from Codacy

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhess-swl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhess-swl Jun 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov bot commented May 27, 2025 •

edited

Loading

codacy-production bot commented May 27, 2025 •

edited

Loading

mhess-swl Jun 4, 2025 •

edited

Loading