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

[API-3583] Track tx costs #57

Merged
merged 23 commits into from
Dec 6, 2024
Merged

[API-3583] Track tx costs #57

merged 23 commits into from
Dec 6, 2024

Conversation

mattac21
Copy link
Contributor

@mattac21 mattac21 commented Nov 19, 2024

This PR has the tx verifier store tx costs in uusdc in the submitted txs table so that solvers are able to track their actual net profit (profit from orders minus tx costs). To do this, I moved the evm tx price oracle to a more generic oracle package, and removed evm specific wording from it (since really all it did was a coin gecko lookup and then decimals conversion to uusdc). The tx verifier uses this to now calculate the tx price in uusdc.

This also changes the fund rebalancer so that it submits its txs to the submitted txs table so that they can be priced. To do this, a rebalance id is added to the submitted txs table. There is also an issue I encountered where we create approval txs before we actually create the row in the rebalance transactions table, however the approval txs should be linked to that rebalance tx in the submitted txs table. To get around this we create a blank rebalance tx before the approval tx (or return an already created blank rebalance tx, if we didn't submit previously due to gas costs), and then update the blank rebalance tx with the tx hash and amount once the rebalance tx is submitted on chain.

@mattac21 mattac21 requested a review from a team as a code owner November 19, 2024 21:48
@mattac21 mattac21 force-pushed the ma/track-profit-in-db branch from 3cb6336 to 94bb59e Compare November 19, 2024 22:52
dhfang
dhfang previously approved these changes Nov 20, 2024
Copy link
Contributor

@dhfang dhfang left a comment

Choose a reason for hiding this comment

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

LGTM! I dont think we put fund rebalancer txs in the submitted txs table right for the tx verifier. Down for you or someone else to add that in a separate pr if you want to merge this.

fundrebalancer/fundrebalancer.go Outdated Show resolved Hide resolved
fundrebalancer/fundrebalancer.go Outdated Show resolved Hide resolved
@mattac21 mattac21 requested review from dhfang and a team December 2, 2024 21:51
fundrebalancer/fundrebalancer.go Outdated Show resolved Hide resolved
shared/oracle/tx_price_oracle.go Show resolved Hide resolved
txverifier/txverifier.go Show resolved Hide resolved
nadim-az
nadim-az previously approved these changes Dec 4, 2024
Copy link
Contributor

@nadim-az nadim-az left a comment

Choose a reason for hiding this comment

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

Changes lgtm, but the creation of the empty rebalance tx in the submitted txs table so it's available for the approval tx to link to is kind of confusing. Can we instead update the approval tx instead after the rebalance tx is created? that would be simpler to follow imo

@mattac21
Copy link
Contributor Author

mattac21 commented Dec 4, 2024

Changes lgtm, but the creation of the empty rebalance tx in the submitted txs table so it's available for the approval tx to link to is kind of confusing. Can we instead update the approval tx instead after the rebalance tx is created? that would be simpler to follow imo

agree that would be easier to follow, and I did think about that scenario but iirc I didnt go with that because of the scenario where we may submit an approval tx and then the solver crashes. in this case we would have no way to know that that approval tx in the submitted txs table is meant to go with the rebalance once the solver starts back up and the solver submits the actual rebalance tx. if you have some ideas on a good way around that tho i would be good to implement that

@nadim-az
Copy link
Contributor

nadim-az commented Dec 4, 2024

Changes lgtm, but the creation of the empty rebalance tx in the submitted txs table so it's available for the approval tx to link to is kind of confusing. Can we instead update the approval tx instead after the rebalance tx is created? that would be simpler to follow imo

agree that would be easier to follow, and I did think about that scenario but iirc I didnt go with that because of the scenario where we may submit an approval tx and then the solver crashes. in this case we would have no way to know that that approval tx in the submitted txs table is meant to go with the rebalance once the solver starts back up and the solver submits the actual rebalance tx. if you have some ideas on a good way around that tho i would be good to implement that

uhh i spent some time on this and any solutions I came up with would also just add complexity, what you have now is the simplest approach i think. thanks! 🙏

shared/bridges/cctp/cosmos_bridge_client.go Outdated Show resolved Hide resolved
shared/bridges/cctp/cosmos_bridge_client.go Show resolved Hide resolved
shared/oracle/tx_price_oracle.go Show resolved Hide resolved
fundrebalancer/fundrebalancer.go Outdated Show resolved Hide resolved
dhfang
dhfang previously approved these changes Dec 5, 2024
Copy link
Contributor

@dhfang dhfang left a comment

Choose a reason for hiding this comment

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

lgtm!

nadim-az
nadim-az previously approved these changes Dec 5, 2024
@mattac21 mattac21 merged commit b825fb4 into main Dec 6, 2024
5 checks passed
@mattac21 mattac21 deleted the ma/track-profit-in-db branch December 6, 2024 19:07
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