-
Notifications
You must be signed in to change notification settings - Fork 115
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor hardcoded id of system account in web3 tests #10448
Conversation
Signed-off-by: filev94 <[email protected]>
Signed-off-by: filev94 <[email protected]>
Signed-off-by: filev94 <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅ |
...irror-web3/src/test/java/com/hedera/mirror/web3/service/AbstractContractCallServiceTest.java
Outdated
Show resolved
Hide resolved
Signed-off-by: filev94 <[email protected]>
Signed-off-by: filev94 <[email protected]>
@@ -54,7 +54,8 @@ void testSuccessfulExecute() throws Exception { | |||
|
|||
@Test | |||
void testExecuteWithInvalidOwner() { | |||
final var systemAccountAddress = toAddress(700); | |||
final var systemAccountId = generateSystemAccountId(); |
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.
I just checked how the domainBuilder.id() method works and it is the following.
/**
* Generates an encoded entity id above the max reserved id 1000. Use {@link #number()} instead for a number
* starting from 1.
*
* @return The generated encoded entity id.
*/
public long id() {
return id.incrementAndGet() + LAST_RESERVED_ID;
}
Looks like the domainBuilder auto generates ids but from 1000 -> upwards, so we shouldn't really be concerned about collisions if our custom id is below 1000. I'm pretty confident thats the case. So we might even close the PR without making any changes to this since it probably isn't a problem
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.
@bilyana-gospodinova, please share your opinion on the case - if we should continue with the change or close the PR.
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.
I double checked and I think Kris is right - the automatic ids that the domain builder generates start from 1000, so there shouldn't be a case where a collision might occur (which was the issue that we were trying to fix).
Closing this PR as per comment regarding the domainBuilder ids. Change is not needed. |
Description:
This PR refactors the hardcoded id of 700 for system account and instead generates one between 1 to 750 inclusive with the exception of 2 and 98 which i found to be special ids.
Related issue(s):
Fixes #10407
Notes for reviewer:
Checklist