web3j icon indicating copy to clipboard operation
web3j copied to clipboard

Why are those decode methods in class TypeDecoder not public?

Open wqbill opened this issue 3 years ago • 1 comments

    @SuppressWarnings("unchecked")
    static <T extends Type> T decode(String input, int offset, Class<T> type) {
        if (NumericType.class.isAssignableFrom(type)) {
            return (T) decodeNumeric(input.substring(offset), (Class<NumericType>) type);
        } else if (Address.class.isAssignableFrom(type)) {
            return (T) decodeAddress(input.substring(offset));
        } else if (Bool.class.isAssignableFrom(type)) {
            return (T) decodeBool(input, offset);
        } else if (Bytes.class.isAssignableFrom(type)) {
            return (T) decodeBytes(input, offset, (Class<Bytes>) type);
        } else if (DynamicBytes.class.isAssignableFrom(type)) {
            return (T) decodeDynamicBytes(input, offset);
        } else if (Utf8String.class.isAssignableFrom(type)) {
            return (T) decodeUtf8String(input, offset);
        } else if (Array.class.isAssignableFrom(type)) {
            throw new UnsupportedOperationException(
                    "Array types must be wrapped in a TypeReference");
        } else {
            throw new UnsupportedOperationException("Type cannot be encoded: " + type.getClass());
        }
    }

I have to use reflection when I want to decode the input data of an transaction. https://github.com/web3j/web3j/issues/489

    private <T extends Type> T decode(String input, int offset, Class<T> type) {
        try {
            Method refMethod = TypeDecoder.class.getDeclaredMethod("decode", String.class, int.class, Class.class);
            refMethod.setAccessible(true);
            return (T) refMethod.invoke(null, input, offset, type);
        } catch (Exception e) {
            throw new UnsupportedOperationException("Type cannot be encoded: " + type.getClass());
        }
    }

Or create a new class under the same package.

package org.web3j.abi;

import org.web3j.abi.datatypes.Type;

public class TypeDecoderMethodExposer {
    public static <T extends Type> T decode(String input, int offset, Class<T> type){
        return TypeDecoder.decode(input, offset, type);
    }
}

But neither of them are elegant. What's the purpose of making those methods non-public?

wqbill avatar Jan 02 '22 13:01 wqbill

Same question here...

linxux avatar Feb 23 '22 17:02 linxux

Same question. It's pretty logical to make such utility method public

PhantomYdn avatar Sep 29 '22 03:09 PhantomYdn

Same here, it seems the library is aggressively hiding things with the intended use case of dApps that write, but don't really read much.

sanguivore-easyco avatar Oct 11 '22 17:10 sanguivore-easyco

I agree with the usefulness of having an accessible decode utility. I'm assuming the design decision behind keeping TypeDecoder and TypeEncoder package protected is intended, to keep them encapsulated inside DefultFunctionlEncoder and DefaultFunctionalReturnDecoder. This is likely to deal with future variations of ABI specification.

We can look into introduction ABI utils package for that purpose.

mohamedelshami avatar Oct 21 '22 17:10 mohamedelshami

FYI, https://github.com/web3j/web3j/pull/1782 is a fix, please comment or +1 there.

monperrus avatar Oct 27 '22 08:10 monperrus

@monperrus @monperrus my recommendation would still be to create a util class that extends FunctionalEncoder and DefaultFunctionalReturnDecoder to provide high-level shorthand APIs for these ABI decode/encode methods.

mohamedelshami avatar Nov 02 '22 08:11 mohamedelshami

Even nicer would be a similar interface that handles the offsets nicely, maybe something of the shape:

public class GenericAbiHandler {
  public GenericAbiHandler(String input) {/*...*/}
  public <T extends Type> T decodeNext(TypeReference<T> type) {/*...*/}
  public static String encode(Collection<TypeReference<?>> types, Object... data) {/*...*/}
}

Possibly spread across two classes instead, one for encoding, one for decoding.

The point being that the ABI encoding/decoding as it stands could be considered in a similar way to JSON, just a way of encoding/decoding data that should be available free of the current concrete "conveniences" for common uses.

sanguivore-easyco avatar Nov 02 '22 16:11 sanguivore-easyco

I like your ideas @mohamedelshami @sanguivore-easyco, we need a better high-level API.

Yet, this PR alone is valuable and fully backward compatible, I would suggest to merge it first and continue later with more heavyweight engineering.

monperrus avatar Nov 03 '22 08:11 monperrus

@monperrus happy to be pragmatic here and merge this for now, can you please add some documentation to what we've discussed about risk of breaking future compatibility and reference this issue in the notes?

mohamedelshami avatar Nov 03 '22 08:11 mohamedelshami

@mohamedelshami sure! See https://github.com/web3j/web3j/pull/1782/commits/de2a890395a3ad30d85c6139c71bc2e7ddb79871

monperrus avatar Nov 03 '22 08:11 monperrus

Thanks, @monperrus. Can you please make that comment more explicit?

e.g. "The public static methods can break future compatibility, and will likely replaced with high level util APIs"

mohamedelshami avatar Nov 03 '22 08:11 mohamedelshami

For the record, methods decode* will be public as of v4.9.5.

Thanks all for your inputs.

monperrus avatar Nov 14 '22 16:11 monperrus