Big fix for codegen when Array of struct and struct used together #2068
Conversation
tonykwok1992
requested review from
conor10,
gtebrean and
NickSneo
as code owners
| return ((internalType == null ? type : internalType.isEmpty() ? type : internalType) | ||
| + components.stream() | ||
| .map(namedType -> String.valueOf(namedType.structIdentifier())) | ||
| .collect(Collectors.joining())) | ||
| .hashCode(); |
There was a problem hiding this comment.
hashCode does not have uniqueness Property, it could subject to quite significant collision as a identifier
Signed-off-by: tonykwok1992 <[email protected]>
tonykwok1992
force-pushed
the
bugfix-codegen
branch
from
6e75649
to
3d0a0b2
Compare
Signed-off-by: tonykwok1992 <[email protected]>
tonykwok1992
force-pushed
the
bugfix-codegen
branch
from
1363404
to
5c49ed3
Compare
Signed-off-by: tonykwok1992 <[email protected]>
| @@ -587,10 +587,6 @@ private List<TypeSpec> buildStructTypes(final List<AbiDefinition> functionDefini | |||
| private static String getStructName(String internalType) { | |||
| final String fullStructName = internalType.substring(internalType.lastIndexOf(" ") + 1); | |||
| String tempStructName = fullStructName.substring(fullStructName.lastIndexOf(".") + 1); | |||
| int arrayPos = tempStructName.indexOf("["); | |||
There was a problem hiding this comment.
This was added in my previous PR merged https://github.com/hyperledger/web3j/pull/2061/files#diff-11944d4d901c6c86e0bb4f4bb1af18f9b3644a325a39e7cc7437878613330294R590
However, after fixing normalizeNamedType to handle static array, this is no longer needed, although there is no harm to leave it there
| @@ -637,7 +646,7 @@ private List<AbiDefinition.NamedType> extractStructs( | |||
| parameters.addAll(definition.getOutputs()); | |||
| return parameters.stream() | |||
| .map(this::normalizeNamedType) | |||
| .filter(namedType -> namedType.getType().startsWith("tuple")); | |||
There was a problem hiding this comment.
This was added in my previous PR merged https://github.com/hyperledger/web3j/pull/2061/files#diff-11944d4d901c6c86e0bb4f4bb1af18f9b3644a325a39e7cc7437878613330294R590
However, after fixing normalizeNamedType to handle static array, this is no longer needed, although there is no harm to leave it there
| @@ -620,6 +617,18 @@ private NamedType normalizeNamedType(NamedType namedType) { | |||
| .getInternalType() | |||
| .substring(0, namedType.getInternalType().length() - 2), | |||
| namedType.isIndexed()); | |||
| } else if (namedType.getType().startsWith("tuple[") | |||
There was a problem hiding this comment.
Main change is to add handling for static array in normalizedType
tonykwok1992
force-pushed
the
bugfix-codegen
branch
from
2ed2336
to
3085bf1
Compare
Signed-off-by: tonykwok1992 <[email protected]>
tonykwok1992
force-pushed
the
bugfix-codegen
branch
from
f04562d
to
a77cc2b
Compare
|
Looks good from my side and also we really apreciate the effort! I'm not approving it yet because I want also to do some checks to make sure, but it should be fine as it is. |
What does this PR do?
After fixing #2061, there is still an edge case where array of struct and struct are used together in the same contract e.g
FooandFoo[2]. Fix this by havingnormalizeNamedTypeto take care of static arrayAlso, structIdentifer is using hashCode which does not have uniqueness Property, it could subject to quite significant collision.
This should also fix #2056. Added test case for the example in the issue
Where should the reviewer start?
All files
Why is it needed?
Bug fix
Checklist