Skip to content
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

Add a condition on the duplicated event names #1872

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

mattiamonari
Copy link
Contributor

What does this PR do?

Add a condition on the name of the events of the java wrapper

Where should the reviewer start?

There is just a one file.

Why is it needed?

Based on Issue #1781, for contracts which have 'overloaded' events (same name, but different signatures) the Java Wrapper creates those events as different instances of the Event class, but with the same variable name, thus resulting in an error. This PR adds the changeEventsName() method which uses an HashMap to count the occurences of an event name, and for those with more than one occurence, it will change the name putting a number at its end, e.g.

public static final Event ACTIONPAUSED_EVENT = new Event("ActionPaused",
            Arrays.<TypeReference<?>>asList(new TypeReference<Utf8String>() {}, new TypeReference<Bool>() {}));
    ;

    public static final Event ACTIONPAUSED1_EVENT = new Event("ActionPaused", 
            Arrays.<TypeReference<?>>asList(new TypeReference<Address>() {}, new TypeReference<Utf8String>() {}, new TypeReference<Bool>() {}));
    ;

@mattiamonari mattiamonari changed the title Add a condition on the duplicated event names #1781 Add a condition on the duplicated event names Mar 3, 2023
Copy link
Contributor

@gtebrean gtebrean left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good workaround for now. Please add also a unit test to can include it in today release.

@mattiamonari
Copy link
Contributor Author

Added the unit test. Not sure if that's the proper way of doing it, please check. I had to put the buildFunctionDefinitions package-private since it was not accessible inside tests.

I also removed the usage of changeEventsName() in buildFuncNameConstants(), l.742 since the function operate just on TYPE_FUNCTION objects (correct me if I'm wrong).

@mattiamonari
Copy link
Contributor Author

Apparently my Unit Test is not working. On my local machine, anyway, it correctly builds and passes tests.

+ " java.util.Arrays.<org.web3j.abi.TypeReference<?>>asList(new org.web3j.abi.TypeReference<org.web3j.abi.datatypes.Utf8String>() {}, new org.web3j.abi.TypeReference<org.web3j.abi.datatypes.Bool>() {}));\n"
+ " ;\n"
+ "\n"
+ " public static final org.web3j.abi.datatypes.Event EVENTNAME1_EVENT = new org.web3j.abi.datatypes.Event(\"eventName1\", \n"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Event name shouldn't be modified new org.web3j.abi.datatypes.Event(\"eventName1\"... will fail in request.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please give me some more details about that?
Do you mean it is not possible to use the abiDef.setName() function on TYPE_EVENTS objects?

Thanks

@gtebrean
Copy link
Contributor

gtebrean commented Mar 3, 2023

I won't add it to this release as it affects the wrapper and has to be carefully tested also by us.
We keep the PR open for finding a better solution.

@mattiamonari
Copy link
Contributor Author

@gtebrean can you please give me some more information if you have them? With the new commit the eventName passed in the constructor should be the same. In my local env. the tests passed, but apparently there's still something which I'm doing wrongly since the checks failed.

@gtebrean
Copy link
Contributor

@gtebrean can you please give me some more information if you have them? With the new commit the eventName passed in the constructor should be the same. In my local env. the tests passed, but apparently there's still something which I'm doing wrongly since the checks failed.

jobs are complaining that:
org.web3j.codegen.SolidityFunctionWrapperTest > testBuildFunctionDuplicatedEventNames() FAILED
org.opentest4j.AssertionFailedError at SolidityFunctionWrapperTest.java:1038.

It fails to create the wrapper, make sure you have latest changes from master.

@mattiamonari
Copy link
Contributor Author

mattiamonari commented Mar 29, 2023

@gtebrean can you please give me some more information if you have them? With the new commit the eventName passed in the constructor should be the same. In my local env. the tests passed, but apparently there's still something which I'm doing wrongly since the checks failed.

jobs are complaining that: org.web3j.codegen.SolidityFunctionWrapperTest > testBuildFunctionDuplicatedEventNames() FAILED org.opentest4j.AssertionFailedError at SolidityFunctionWrapperTest.java:1038.

It fails to create the wrapper, make sure you have latest changes from master.

My branch when I commited was up-to-date with master. The method org.web3j.codegen.SolidityFunctionWrapperTest > testBuildFunctionDuplicatedEventNames() is the unit test I created my self for this PR, but in my local env is passing the test, whereas in the jobs it's not. Which could be the cause? Error logs aren't really useful.

@stale
Copy link

stale bot commented Jun 18, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale no activity for 21 days label Jun 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale no activity for 21 days
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants