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

Fix smart contract code generators using #671

Closed
conor10 opened this issue Aug 10, 2018 · 6 comments
Closed

Fix smart contract code generators using #671

conor10 opened this issue Aug 10, 2018 · 6 comments
Labels
bug A bug in behaviour or functionality enhancement a feature request help wanted

Comments

@conor10
Copy link
Contributor

conor10 commented Aug 10, 2018

With the gas provider changes made to web3j, there are issues with smart contract wrappers missing additional constructors.

Currently smart contract wrappers only have two generated constructors (example taken from HumanStandardToken:

    protected HumanStandardToken(String contractAddress, Web3j web3j, Credentials credentials, BigInteger gasPrice, BigInteger gasLimit) {
        super(BINARY, contractAddress, web3j, credentials, gasPrice, gasLimit);
    }

    protected HumanStandardToken(String contractAddress, Web3j web3j, TransactionManager transactionManager, BigInteger gasPrice, BigInteger gasLimit) {
        super(BINARY, contractAddress, web3j, transactionManager, gasPrice, gasLimit);
    }

You need to update the SolidityFunctionWrapper to generate the following methods in addition to the above (example using HumanStandardToken generated code - see build.sh):

    protected HumanStandardToken(String contractAddress, Web3j web3j, Credentials credentials, ContractGasProvider gasProvider) {
        super(BINARY, contractAddress, web3j, credentials, gasProvider);
    }

    protected HumanStandardToken(String contractAddress, Web3j web3j, TransactionManager transactionManager, ContractGasProvider gasProvider) {
        super(BINARY, contractAddress, web3j, transactionManager, gasProvider);
    }

The following constructor should also be added to the Contract class:

    protected Contract(String contractBinary, String contractAddress,
                       Web3j web3j, TransactionManager transactionManager,
                       ContractGasProvider gasProvider) {
                this(contractBinary, contractAddress, web3j, new RawTransactionManager(web3j, credentials), gasProvider);
    }
@conor10 conor10 added bug A bug in behaviour or functionality enhancement a feature request help wanted labels Aug 10, 2018
@franz-see
Copy link

@conor10 Added a pull request to fix address issues - #672

@conor10
Copy link
Contributor Author

conor10 commented Aug 13, 2018

Thanks!

There were some details I realised I should have included:

The deploy<contractName> and load<contractName methods should also be updated to use the defaultGasProvider.

This has made me think that it's probably better to remove the deprecated constructors for generated contracts, and just use the defaultGasProvider everywhere.

@franz-see
Copy link

@conor10 Thanks for the feedback. Can you expound on your comments?

Comment (a.)

The deploy<contractName> and load<contractName> methods should also be updated to use the defaultGasProvider.

Comment (b.)

This has made me think that it's probably better to remove the deprecated constructors for generated contracts, and just use the defaultGasProvider everywhere.

.
.
.

Comment (a.) - The deploy<contractName> and load<contractName> methods should also be updated to use the defaultGasProvider.

I dont see any deploy and load methods, but I do see the following:

(example, from HumanStandardToken)

    public static RemoteCall<HumanStandardToken> deploy(
            Web3j web3j, 
            Credentials credentials, 
            BigInteger gasPrice, 
            BigInteger gasLimit, 
            BigInteger _initialAmount, 
            String _tokenName, 
            BigInteger _decimalUnits, 
            String _tokenSymbol) {
        ...
    }

    public static RemoteCall<HumanStandardToken> deploy(
            Web3j web3j, 
            TransactionManager transactionManager, 
            BigInteger gasPrice, 
            BigInteger gasLimit, 
            BigInteger _initialAmount, 
            String _tokenName, 
            BigInteger _decimalUnits, 
            String _tokenSymbol) {
        ...
    }

    public static HumanStandardToken load(
            String contractAddress, 
            Web3j web3j, 
            Credentials credentials, 
            BigInteger gasPrice, 
            BigInteger gasLimit) {
        ...
    }

    public static HumanStandardToken load(
            String contractAddress, 
            Web3j web3j, 
            TransactionManager transactionManager, 
            BigInteger gasPrice, 
            BigInteger gasLimit) {
        ...
    }

Do you mean deprecate those methods above and add methods that use ContractGasProvider instead?

Something like...

    @Deprecated
    public static RemoteCall<HumanStandardToken> deploy(
            Web3j web3j, 
            Credentials credentials, 
            BigInteger gasPrice, 
            BigInteger gasLimit, 
            BigInteger _initialAmount, 
            String _tokenName, 
            BigInteger _decimalUnits, 
            String _tokenSymbol) {
        ...
    }

    @Deprecated
    public static RemoteCall<HumanStandardToken> deploy(
            Web3j web3j, 
            TransactionManager transactionManager, 
            BigInteger gasPrice, 
            BigInteger gasLimit, 
            BigInteger _initialAmount, 
            String _tokenName, 
            BigInteger _decimalUnits, 
            String _tokenSymbol) {
        ...
    }

    @Deprecated
    public static HumanStandardToken load(
            String contractAddress, 
            Web3j web3j, 
            Credentials credentials, 
            BigInteger gasPrice, 
            BigInteger gasLimit) {
        ...
    }

    @Deprecated
    public static HumanStandardToken load(
            String contractAddress, 
            Web3j web3j, 
            TransactionManager transactionManager, 
            BigInteger gasPrice, 
            BigInteger gasLimit) {
        ...
    }

    public static RemoteCall<HumanStandardToken> deploy(
            Web3j web3j, 
            Credentials credentials, 
            ContractGasProvider gasProvider,
            BigInteger _initialAmount, 
            String _tokenName, 
            BigInteger _decimalUnits, 
            String _tokenSymbol) {
        ...
    }

    public static RemoteCall<HumanStandardToken> deploy(
            Web3j web3j, 
            TransactionManager transactionManager, 
            ContractGasProvider gasProvider, 
            BigInteger _initialAmount, 
            String _tokenName, 
            BigInteger _decimalUnits, 
            String _tokenSymbol) {
        ...
    }

    public static HumanStandardToken load(
            String contractAddress, 
            Web3j web3j, 
            Credentials credentials, 
            ContractGasProvider gasProvider) {
        ...
    }

    @Deprecated
    public static HumanStandardToken load(
            String contractAddress, 
            Web3j web3j, 
            TransactionManager transactionManager, 
            ContractGasProvider gasProvider) {
        ...
    }

(Also, Im guessing by defaultGasProvider, you're referring to ContractGasProvider)

.
.
.

Comment (b.) - This has made me think that it's probably better to remove the deprecated constructors for generated contracts, and just use the defaultGasProvider everywhere.

There are only 2 constructors on the generated classes

    protected HumanStandardToken(String contractAddress, Web3j web3j, Credentials credentials, BigInteger gasPrice, BigInteger gasLimit) {
        ...
    }

    protected HumanStandardToken(String contractAddress, Web3j web3j, TransactionManager transactionManager, BigInteger gasPrice, BigInteger gasLimit) {
        ...
    }

There are no deprecated constructors in the generated contracts. Or do you mean to deprecate the constructors that use gasPrice and gasLimit and add 2 more constructors that use ContractGasProvider instead?

Something like ...

    @Deprecated
    protected HumanStandardToken(String contractAddress, Web3j web3j, Credentials credentials, BigInteger gasPrice, BigInteger gasLimit) {
        ...
    }

    @Deprecated
    protected HumanStandardToken(String contractAddress, Web3j web3j, TransactionManager transactionManager, BigInteger gasPrice, BigInteger gasLimit) {
        ...
    }

    protected HumanStandardToken(String contractAddress, Web3j web3j, Credentials credentials, ContractGasProvider gasProvider) {
        ...
    }

    protected HumanStandardToken(String contractAddress, Web3j web3j, TransactionManager transactionManager, ContractGasProvider gasProvider) {
        ...
    }

Thanks!

@yuriymyronovych
Copy link
Contributor

Sorry guys, saw this issue only now, @franz-see let me know if you will need any help with that fix(I will check PR now). I feel responsible for that feature :)

@franz-see
Copy link

@yuriymyronovych The pull request for this is still valid (no conflicts). But @conor10 did have an additional comment that I needed verification on.

Kindly see #671 (comment) for the clarifications that I need :)

@yuriymyronovych
Copy link
Contributor

@franz-see I believe @conor10 meant to simply remove gas limit/gas price from everywhere(constructors & deploy/load) and just use GasPriceProvider, it would totally make sense to me, there will be 4.0 anyway with quite a few of new api changes.

It seems they are a bit overwhelmed right now, lets give them time to come back on this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A bug in behaviour or functionality enhancement a feature request help wanted
Projects
None yet
Development

No branches or pull requests

4 participants