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

feat(abstract-eth): fetch gasPrice and gasLimit from explorer #4905

Conversation

venkateshv1266
Copy link
Contributor

TICKET: COIN-1201

@venkateshv1266 venkateshv1266 force-pushed the COIN-1201-wrw-update-fields-to-get-gas-fees-and-gasLimit-from-explorer branch from 8a9ee61 to 3a75ae4 Compare September 12, 2024 11:42
@venkateshv1266 venkateshv1266 changed the title feat(ccr): get gasPrice and gasLimit from explorer feat(abstract-eth): fetch gasPrice and gasLimit from explorer Sep 12, 2024
@venkateshv1266 venkateshv1266 force-pushed the COIN-1201-wrw-update-fields-to-get-gas-fees-and-gasLimit-from-explorer branch 2 times, most recently from ab0299b to 6e784b3 Compare September 12, 2024 12:10
@venkateshv1266 venkateshv1266 marked this pull request as ready for review September 12, 2024 12:40
@venkateshv1266 venkateshv1266 requested a review from a team as a code owner September 12, 2024 12:40
@@ -1520,10 +1520,14 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin {
}
}

const gasLimit = new optionalDeps.ethUtil.BN(this.setGasLimit(params.gasLimit));
// If gasLimit is not passed as a param, then default gasLimit 50000 will be set here
let gasLimit = new optionalDeps.ethUtil.BN(this.setGasLimit(params.gasLimit));
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is needed, we can do the balance check after we get the correct gasLimit
also we shouldn't pass any default values for fee and gasLimit from wrw

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, actually we are not passing any default values from WRW.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the balance check after we get the gasLimit from explorer

@venkateshv1266 venkateshv1266 force-pushed the COIN-1201-wrw-update-fields-to-get-gas-fees-and-gasLimit-from-explorer branch 2 times, most recently from 56e528b to c9afb5e Compare September 13, 2024 10:38
@venkateshv1266 venkateshv1266 force-pushed the COIN-1201-wrw-update-fields-to-get-gas-fees-and-gasLimit-from-explorer branch from c9afb5e to 2a8f36e Compare September 13, 2024 10:48
@@ -1520,25 +1520,20 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin {
}
}

const gasLimit = new optionalDeps.ethUtil.BN(this.setGasLimit(params.gasLimit));
// Use default gasLimit for cold and custody wallets
let gasLimit =
Copy link
Contributor

Choose a reason for hiding this comment

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

actually is gasLimit mandatory for tx building if not we can just fetch it at the WP

Copy link
Contributor Author

@venkateshv1266 venkateshv1266 Sep 13, 2024

Choose a reason for hiding this comment

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

yes, it is mandatory to pass gasLimit... If not, it will throw an error while building the txn Error: Cannot convert string to buffer. toBuffer only supports 0x-prefixed hex strings and this string was given: 0xNaN

@venkateshv1266 venkateshv1266 merged commit a05dd50 into master Sep 13, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants