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

[IND-416] Add default fee for v4 python client #54

Merged
merged 5 commits into from
Oct 13, 2023
Merged

Conversation

dydxwill
Copy link
Contributor

No description provided.

@@ -18,6 +18,7 @@ def prepare_and_broadcast_basic_transaction(
gas_limit: Optional[int] = None,
memo: Optional[str] = None,
broadcast_mode: BroadcastMode = None,
fee: Optional[str] = "5000dv4tnt",
Copy link
Contributor

@Christopher-Li Christopher-Li Oct 11, 2023

Choose a reason for hiding this comment

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

Kind of open questions, but shouldn't fee be required? I think on the protocol it is required. Additionally on mainnet will the token be called dv4tnt? Not sure if we want to default to this value.

Copy link
Contributor

Choose a reason for hiding this comment

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

Placing orders does not require a fee.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

set the fee to be 0 if placing an order. We can change the default token when mainnet comes around?

Copy link
Contributor

@vincentwschau vincentwschau Oct 12, 2023

Choose a reason for hiding this comment

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

May make more sense to define a default coin based on the network in the constants here, and take in both the amount and the coin for the fee in this function, with the defaults set to the 5000 and the coin in the constants file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -96,6 +96,9 @@

BECH32_PREFIX = 'dydx'

# ------------ DEFAULT TOKEN ------------
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Change the comment to indicate it's the testnet token.

@vincentwschau
Copy link
Contributor

Can we set the fee_denomination here and use that network config in some way instead?

@@ -93,7 +108,7 @@ def fetchai_stable_testnet(cls):

:return: fetchai stable testnet. For now dorado is fetchai stable testnet.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Update this comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -28,6 +28,8 @@ def prepare_and_broadcast_basic_transaction(
:param account: The account
:param gas_limit: The gas limit
:param memo: Transaction memo, defaults to None
:param broadcast_mode: Broadcast mode, defaults to None
:param fee: Transaction fee, defaults to 5000
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Note that the denomination is from the network config.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@dydxwill dydxwill merged commit 5f388cf into main Oct 13, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants