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

Utilize gas estimation in client libraries #173

Conversation

aefhm
Copy link
Contributor

@aefhm aefhm commented Sep 4, 2023

Why

Relates to #39

Development

Go

This appears fairly straight forward. I can test with a base Go script against our Vigil contract.

package main

import (
	"context"
	"crypto/ecdsa"
	"fmt"
	"log"
	"math/big"

	"github.com/aefhm/vigil" // or however you do local modules
	"github.com/ethereum/go-ethereum/accounts/abi/bind"
	"github.com/ethereum/go-ethereum/crypto"
	"github.com/ethereum/go-ethereum/ethclient"
	sapphire "github.com/oasisprotocol/sapphire-paratime/clients/go"
)

var SapphireChainID = big.NewInt(0x5aff)

func main() {
	c, err := ethclient.Dial("https://testnet.sapphire.oasis.dev")
	if err != nil {
		log.Fatal(err)
	}

	privateKey, err := crypto.HexToECDSA("")
	if err != nil {
		log.Fatal(err)
	}

	publicKey := privateKey.Public()
	publicKeyECDSA, ok := publicKey.(*ecdsa.PublicKey)
	if !ok {
		log.Fatal("error casting public key to ECDSA")
	}

	var client *sapphire.WrappedBackend
	client, err = sapphire.WrapClient(*c, func(digest [32]byte) ([]byte, error) { return crypto.Sign(digest[:], privateKey) })

	if err != nil {
		log.Fatal(err)
	}

	fromAddress := crypto.PubkeyToAddress(*publicKeyECDSA)
	nonce, err := client.PendingNonceAt(context.Background(), fromAddress)

	gasPrice, err := client.SuggestGasPrice(context.Background())
	if err != nil {
		log.Fatal(err)
	}

	auth, _ := bind.NewKeyedTransactorWithChainID(privateKey, SapphireChainID)
	auth.Nonce = big.NewInt(int64(nonce))
	auth.Value = big.NewInt(0) // in wei
	auth.GasLimit = uint64(0)  // in units
	auth.GasPrice = gasPrice

	if err != nil {
		log.Fatal(err)
	}

	address, tx, _, err := vigil.DeployVigil(auth, client)
	if err != nil {
		log.Fatal(err)
	}

	fmt.Println(address.Hex())
	fmt.Println(tx.Hash().Hex())


        // create secret
        // reuse address or wait
	vigil, err := vigil.NewVigil(address, client)

	if err != nil {
		log.Fatalf("Failed to instantiate Vigil contract: %v", err)
	}

	val := []byte("123")
	transactOpts := client.Transactor(fromAddress)
	transaction, err := vigil.CreateSecret(transactOpts, "age", big.NewInt(5), val)

	if err != nil {
		log.Fatalf("Failed to create secret: %v", err)
	}

	fmt.Println(transaction.Data())
}

JS

I'm testing with Vigil.sol and run-vigil.ts

TODO

  • Inform community
  • Test locally again

@aefhm aefhm self-assigned this Sep 4, 2023
@aefhm aefhm added p:2 Priority: nice to have s:underway Status: a patch is in progress go Pull requests that update Go code js labels Sep 4, 2023
@aefhm aefhm force-pushed the CU-86785j9hu_Dont-return-hard-coded-gas-estimation-in-Sapphire-Client-Library_Xi-Zhang branch 5 times, most recently from 9d98834 to 237fa96 Compare September 5, 2023 06:08
@aefhm aefhm marked this pull request as ready for review September 5, 2023 16:23
@aefhm
Copy link
Contributor Author

aefhm commented Sep 7, 2023

Hmm, looks like Truffle integration gas estimates are off.

estimate: 159564,
  error: Error: The transaction receipt didn't contain a contract address.
      at /Users/xi/oasis/sapphire-paratime/node_modules/.pnpm/[email protected]/node_modules/truffle/build/webpack:/packages/deployer/src/deployment.js:302:1
      at processTicksAndRejections (node:internal/process/task_queues:96:5) {
    receipt: {
      blockHash: '0xe02b3d5f3476d46ffb44ebc536f876f8996702fb5919efca4784e8ab295e7192',
      blockNumber: 99,
      contractAddress: null,
      cumulativeGasUsed: 199455,
      from: '0xa19875987799b8b52150E2a718F338FA36d8981d',
      gasUsed: 199455,
      logs: [],
      logsBloom: '0x00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000',
      status: false,
      to: null,
      transactionHash: '0x040881a41eeba57a98466e7ff99611d056fb41df8f801515d695116efca722ba',
      transactionIndex: 0,
      type: '0x0'
    },

@CedarMist CedarMist force-pushed the CU-86785j9hu_Dont-return-hard-coded-gas-estimation-in-Sapphire-Client-Library_Xi-Zhang branch from 237fa96 to 04c94cf Compare September 18, 2023 17:20
@aefhm aefhm force-pushed the CU-86785j9hu_Dont-return-hard-coded-gas-estimation-in-Sapphire-Client-Library_Xi-Zhang branch from 04c94cf to a9fcd7a Compare September 19, 2023 10:06
@CedarMist
Copy link
Member

CedarMist commented Sep 21, 2023

@aefhm Lets remove Truffle support, that will unblock this :D

https://consensys.io/blog/consensys-announces-the-sunset-of-truffle-and-ganache-and-new-hardhat

Starting on December 20, 2023, Truffle and Ganache codebases will remain available as public archives. This gives developers around 90 days to migrate to HardHat and other solutions.

@aefhm
Copy link
Contributor Author

aefhm commented Sep 24, 2023

@aefhm Lets remove Truffle support, that will unblock this :D

https://consensys.io/blog/consensys-announces-the-sunset-of-truffle-and-ganache-and-new-hardhat

Starting on December 20, 2023, Truffle and Ganache codebases will remain available as public archives. This gives developers around 90 days to migrate to HardHat and other solutions.

I'm game. Will remove Truffle in a separate PR/issue and inform the community.

@aefhm aefhm added the s:blocked Status: work on this issue is blocked label Sep 24, 2023
@aefhm
Copy link
Contributor Author

aefhm commented Sep 25, 2023

Relates to oasisprotocol/docs#547

@matevz
Copy link
Member

matevz commented Oct 11, 2023

Is gas estimation possible, if we don't sign the gas estimation call? Most of estimates will be off, if the contract checks msg.sender credentials.

@aefhm aefhm force-pushed the CU-86785j9hu_Dont-return-hard-coded-gas-estimation-in-Sapphire-Client-Library_Xi-Zhang branch from a9fcd7a to 344db32 Compare October 12, 2023 17:40
@kostko
Copy link
Member

kostko commented Oct 12, 2023

1 similar comment
@kostko
Copy link
Member

kostko commented Oct 12, 2023

@aefhm aefhm force-pushed the CU-86785j9hu_Dont-return-hard-coded-gas-estimation-in-Sapphire-Client-Library_Xi-Zhang branch from 344db32 to 23f44a0 Compare October 18, 2023 23:49
@aefhm aefhm changed the base branch from main to xz/remove-truffle October 18, 2023 23:51
@aefhm aefhm force-pushed the CU-86785j9hu_Dont-return-hard-coded-gas-estimation-in-Sapphire-Client-Library_Xi-Zhang branch from 23f44a0 to 2e3bf3e Compare October 18, 2023 23:52
@aefhm aefhm force-pushed the xz/remove-truffle branch from ee9bd1e to cb2cead Compare October 20, 2023 15:27
Base automatically changed from xz/remove-truffle to main October 20, 2023 15:32
@aefhm aefhm force-pushed the CU-86785j9hu_Dont-return-hard-coded-gas-estimation-in-Sapphire-Client-Library_Xi-Zhang branch from 2e3bf3e to 85baf43 Compare October 20, 2023 17:01
@aefhm aefhm force-pushed the CU-86785j9hu_Dont-return-hard-coded-gas-estimation-in-Sapphire-Client-Library_Xi-Zhang branch from 85baf43 to 7161b42 Compare November 1, 2023 15:57
@aefhm aefhm force-pushed the CU-86785j9hu_Dont-return-hard-coded-gas-estimation-in-Sapphire-Client-Library_Xi-Zhang branch from 7161b42 to 3af70fd Compare November 1, 2023 16:27
@aefhm
Copy link
Contributor Author

aefhm commented Nov 1, 2023

Is gas estimation possible, if we don't sign the gas estimation call? Most of estimates will be off, if the contract checks msg.sender credentials.

I believe so. I tried testing with some

require(
    msg.sender == _metas[index].creator,
    "only for creator"
);

Most of estimates will be off, if the contract checks msg.sender credentials.

Could you elaborate? 🤔

@CedarMist
Copy link
Member

Gas estimation is possible without signed queries, it's just msg.sender will be address(0).

So if the code relies on signed queries, gas estimation will be wrong.

If the code doesn't rely on signed queries, then gas estimation will be OK.

@aefhm
Copy link
Contributor Author

aefhm commented Nov 2, 2023

Gas estimation is possible without signed queries, it's just msg.sender will be address(0).

👍

So if the code relies on signed queries, gas estimation will be wrong.

Wrong in the safe way though? #39 (comment)

If the code doesn't rely on signed queries, then gas estimation will be OK.

👌

I believe we are ~okay turning gas estimation on with this caveat?

@aefhm aefhm merged commit 9ccaf13 into main Nov 4, 2023
13 checks passed
@aefhm aefhm deleted the CU-86785j9hu_Dont-return-hard-coded-gas-estimation-in-Sapphire-Client-Library_Xi-Zhang branch November 4, 2023 22:01
github-actions bot added a commit that referenced this pull request Nov 4, 2023
…U-86785j9hu_Dont-return-hard-coded-gas-estimation-in-Sapphire-Client-Library_Xi-Zhang

Utilize gas estimation in client libraries 9ccaf13
github-actions bot added a commit that referenced this pull request Nov 4, 2023
…-86785j9hu_Dont-return-hard-coded-gas-estimation-in-Sapphire-Client-Library_Xi-Zhang

Utilize gas estimation in client libraries 9ccaf13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
go Pull requests that update Go code p:2 Priority: nice to have s:blocked Status: work on this issue is blocked s:underway Status: a patch is in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants