Skip to content
This repository has been archived by the owner on Oct 15, 2024. It is now read-only.

feat: add estimatedGas, estimatedGasTokenRefund and minimumProtocolFee to /quote and /price response #237

Merged
merged 26 commits into from
May 29, 2020

Conversation

fragosti
Copy link
Contributor

@fragosti fragosti commented May 20, 2020

Description

  • Add estimatedGas field which represents a more optimistic gas estimate for the UI.
  • Add estimagedGasTokenRefund field which represents an estimate for the gas token refund in the transaction.
  • Add minimumProtocolFee field which represents the protocol fee if only the minimum amount of orders are filled.
  • Did some refactoring as the main method was getting long/messy.

Testing Instructions

It is deployed on staging. https://staging.api.0x.org/swap/v0/quote?sellToken=DAI&buyToken=WETH&buyAmount=1000000000000000000000

Checklist

@fragosti fragosti changed the title [WIP]feat: estimated fields feat: estimated fields May 21, 2020
@fragosti fragosti changed the title feat: estimated fields feat: add estimatedGas, estimatedGasTokenRefund and estimatedProtocolFee to /quote /price and response May 21, 2020
@fragosti
Copy link
Contributor Author

deploy staging

@fragosti
Copy link
Contributor Author

deploy staging

@@ -132,4 +134,30 @@ export const serviceUtils = {
breakdown,
);
},
getEstimatedGasTokenRefundInfo(
orders: OptimizedMarketOrder[],
gasTokenBalance: BigNumber = new BigNumber('100000000000000000000000'),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO: remove.

@fragosti fragosti changed the title feat: add estimatedGas, estimatedGasTokenRefund and estimatedProtocolFee to /quote /price and response feat: add estimatedGas, estimatedGasTokenRefund and estimatedProtocolFee to /quote and /price response May 21, 2020
@fragosti
Copy link
Contributor Author

deploy staging

@fragosti fragosti changed the title feat: add estimatedGas, estimatedGasTokenRefund and estimatedProtocolFee to /quote and /price response feat: add estimatedGas, estimatedGasTokenRefund and minimumProtocolFee to /quote and /price response May 22, 2020
src/constants.ts Outdated
@@ -15,6 +15,7 @@ export const DEFAULT_LOGGER_INCLUDE_TIMESTAMP = true;
export const ONE_SECOND_MS = 1000;
export const ONE_MINUTE_MS = ONE_SECOND_MS * 60;
export const TEN_MINUTES_MS = ONE_MINUTE_MS * 10;
export const GST2_TOKEN_ADDRESS = '0x0000000000b3f879cb30fe243b4dfee438691c04';
Copy link
Contributor

Choose a reason for hiding this comment

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

Might this belong in the @0x/contract-addresses package? What happens on kovan / ganache?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The contract-addresses package mostly contains smart contracts that we own, this is just a token, so put it in the token_metadatas_for_networks file.

src/config.ts Outdated
@@ -205,6 +205,10 @@ export const PROMETHEUS_PORT: number = _.isEmpty(process.env.PROMETHEUS_PORT)
? 8080
: assertEnvVarType('PROMETHEUS_PORT', process.env.PROMETHEUS_PORT, EnvVarType.Port);

export const GST2_WALLET_ADDRESS: string = _.isEmpty(process.env.GST2_WALLET_ADDRESS)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same thing here about multiple networks

Copy link
Contributor Author

@fragosti fragosti May 27, 2020

Choose a reason for hiding this comment

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

Are you saying we make it required? For different networks we can override the deployment.

It's a bit of a weird one because the bridges have an allowance to this address, so it makes sense to have it be baked in.

Copy link
Contributor

Choose a reason for hiding this comment

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

just wanted to call it out, what happens if we forget to override the deployment? Any risk?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nothing would happen but I moved it to a chainId -> string object in constants.ts which probably makes more sense

const usedGasTokens = BigNumber.min(
gasTokenBalance,
costOfBridgeFills
.plus(14154)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should these numbers be in constants somewhere?

Copy link
Member

@dekz dekz left a comment

Choose a reason for hiding this comment

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

LGTM.

In the future It'd be nice to cache the GST balance for some time (1min?) rather than query it live every time. Whilst it may have a delay in accuracy as we approach a 0 balance for that time, but it would be faster (and less RPC calls) for 99.99% of the time where we have a reserve.

@fragosti fragosti merged commit f7f3277 into master May 29, 2020
@fragosti fragosti deleted the feat/estimated-fields branch May 29, 2020 18:45
github-actions bot pushed a commit that referenced this pull request Jun 1, 2020
# [1.6.0](v1.5.0...v1.6.0) (2020-06-01)

### Bug Fixes

* Re-pin asset-swapper to latest monorepo commit ([#240](#240)) ([dfbf0e7](dfbf0e7))

### Features

* add estimatedGas, estimatedGasTokenRefund and minimumProtocolFee to /quote and /price  response ([#237](#237)) ([f7f3277](f7f3277))
@github-actions
Copy link

github-actions bot commented Jun 1, 2020

🎉 This PR is included in version 1.6.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants