Skip to content

Feat/message broadcaster without simulation - #226

Merged
aarmoa merged 19 commits into
masterfrom
feat/message_broadcaster_without_simulation
Aug 24, 2023
Merged

Feat/message broadcaster without simulation#226
aarmoa merged 19 commits into
masterfrom
feat/message_broadcaster_without_simulation

Conversation

@aarmoa

@aarmoa aarmoa commented Aug 18, 2023

Copy link
Copy Markdown
Contributor
  • Added logic to allow the transaction broadcaster to calculate gas limit without simulating the transaction

@aarmoa
aarmoa requested a review from achilleas-kal August 18, 2023 16:10

@achilleas-kal achilleas-kal left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The example as is fails with an out of gas error as per this tx.

It looks like DEFAULT_GAS_LIMIT is not being added and we send the tx simply with DEFAULT_EXCHANGE_GAS_LIMIT (100k). We should not calculate gas per "message" but per "object". Clients send a single message i.e. MsgBatchUpdateOrders but that includes multiple order placements and cancellations.

I also strongly recommend we have more in-depth gas calculation for exchange objects. Order placement with order cancellation have a lot of difference in terms of gas and MMs already pay a lot in gas daily, they'd need to have a more curated calculation instead of paying 100K gas for every exchange interaction.

@achilleas-kal

Copy link
Copy Markdown
Collaborator

Effectively the calculation should include:

  1. Default_gas_limit = gas for the tx itself since fields such as memo and timeout_height cost gas.
  2. Gas per object
  3. You can consider a "buffer" variable than can also be modifiable be the clients so we can have more strict gas per object and have a buffer to account for different ops in IAVL.

…sider the cost of each included element for batch messages
@aarmoa
aarmoa requested a review from achilleas-kal August 22, 2023 13:21

@achilleas-kal achilleas-kal left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thank you @aarmoa.

  1. Could we allow AVERAGE_CANCEL_ALL_AFFECTED_ORDERS to be modifiable by clients separately for spot and derivative orders? I think users should ideally be able to set the number of spot and derivative orders to cancel separately and then we calculate gas based on the constants we have. We could also consider adding a buffer here that can be also overridden by clients to account for transient orders.
  2. There's no need to have support for MsgBatchCancel, MsgBatchCreate on spot/derivatives. We should only have support for MsgBatchUpdateOrders in the future. This message allows them to perform any action on the exchange. The chain team is also making improvements only on this specific message (i.e. error flags on the chain show only for MsgBatchUpdateOrders). We can also remove the examples in the SDK and also API docs. Effectively these are obsolete messages upon introducing MsgBatchUpdateOrders.

@aarmoa

aarmoa commented Aug 23, 2023

Copy link
Copy Markdown
Contributor Author

@achilleas-kal let me see what I can do for point 1.
Regarding point 2, since those messages are still present in the SDK and I have already implemented the code to estimate their gas cost, I don't see the need to remove them now. We can remove them when the messages are no longer present in the SDK.

@aarmoa
aarmoa requested a review from achilleas-kal August 24, 2023 12:10
Morgandri1 and others added 3 commits August 24, 2023 11:24
@aarmoa
aarmoa merged commit 880ba4b into master Aug 24, 2023
@aarmoa
aarmoa deleted the feat/message_broadcaster_without_simulation branch August 24, 2023 16:39
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.

3 participants