-
Notifications
You must be signed in to change notification settings - Fork 22
feat: running balance calculations for optimistic proposals #1142
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
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: bennett <[email protected]>
Signed-off-by: bennett <[email protected]>
Signed-off-by: bennett <[email protected]>
Signed-off-by: bennett <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One q
Signed-off-by: bennett <[email protected]>
Signed-off-by: bennett <[email protected]>
Signed-off-by: Bennett <[email protected]>
Signed-off-by: Bennett <[email protected]>
Signed-off-by: Bennett <[email protected]>
Signed-off-by: Bennett <[email protected]>
Signed-off-by: Bennett <[email protected]>
Signed-off-by: Bennett <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I think this should work... best way to test is to wait until a new root bundle is proposed, then run the proposer locally using the same bundle block ranges.
Then, delete the ExecutedRootBundle
events in the HubPoolClient locally so the just-recently executed bundle doesn't look like a "validated root bundle" and I think the running balances should match.
WDYT of this testing plan? I could be wrong
I think this works. I will try to get some comparisons here and then will bump package one last time. |
Signed-off-by: Bennett <[email protected]>
Signed-off-by: bennett <[email protected]>
Signed-off-by: bennett <[email protected]>
Tested this with the above suggestion. I specifically indexed this by Under this setup, I have been able to reproduce identical bundles as the one actually proposed. |
} | ||
}); | ||
}); | ||
const poolRebalanceLeaves: PoolRebalanceLeaf[] = constructPoolRebalanceLeaves( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: consider refactoring this constructing leaf section into shared function if its clean, but perhaps you've thought of this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought the cleanest way would be to make _buildHistorical...
and _buildOptimistic...
return runningBalances
, chainWithRefundsOnly
, and realizedLpFees
back to _buildPoolRebalanceRoot
so that it can centralize the building there, but the issue I ran into was calling _buildHistoricalPoolRebalanceRoot in _buildOptimisticPoolRebalanceRoot
since that function actually needs to have the tree/pool rebalance leaves. I think that means we would need this function to be called in both _buildOptimisticPoolRebalanceRoot
and _buildHistoricalPoolRebalanceRoot
, and the issue with that is we need to pass in everything constructPoolRebalanceLeaves
needs to have, so it would really just add to the diff by a decent amount.
Signed-off-by: bennett <[email protected]>
We need to know the pending root bundle's running balance totals, which is only retrievable by recomputing the pending root bundle's pool rebalance root.