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

Fix Bridge pool recommendations #1811

Merged
merged 89 commits into from
Sep 6, 2023
Merged

Conversation

sug0
Copy link
Contributor

@sug0 sug0 commented Aug 13, 2023

Describe your changes

Closes #1799

Indicate on which release or other PRs this topic is based on

Based on #1795

Checklist before merging to draft

  • I have added a changelog
  • Git history is in acceptable state

@sug0 sug0 changed the title Tiago/fix bp recommendations Fix Bridge pool recommendations Aug 13, 2023
@sug0 sug0 force-pushed the tiago/fix-bp-recommendations branch from ca9b0d4 to 88dcf90 Compare August 14, 2023 08:45
sug0 added a commit that referenced this pull request Aug 14, 2023
@sug0 sug0 marked this pull request as ready for review August 14, 2023 13:19
Copy link
Member

@batconjurer batconjurer left a comment

Choose a reason for hiding this comment

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

The code changes look fine, but needs to new unit tests to cover the new code paths for provided conversion tables.

@sug0 sug0 marked this pull request as draft August 16, 2023 09:17
@sug0 sug0 force-pushed the tiago/fix-bp-recommendations branch from 628dc99 to 9a29d88 Compare August 16, 2023 14:06
sug0 added a commit that referenced this pull request Aug 16, 2023
@sug0 sug0 force-pushed the tiago/fix-bp-recommendations branch from 9a29d88 to ff93833 Compare August 16, 2023 14:07
sug0 added a commit that referenced this pull request Aug 16, 2023
@sug0 sug0 force-pushed the tiago/fix-bp-recommendations branch from ff93833 to d8ddf6e Compare August 16, 2023 14:08
sug0 added a commit that referenced this pull request Aug 16, 2023
@sug0 sug0 force-pushed the tiago/fix-bp-recommendations branch from d8ddf6e to 83bafe9 Compare August 16, 2023 14:16
sug0 added a commit that referenced this pull request Aug 16, 2023
@sug0 sug0 force-pushed the tiago/fix-bp-recommendations branch from 83bafe9 to 8079c9a Compare August 16, 2023 14:47
@sug0
Copy link
Contributor Author

sug0 commented Aug 16, 2023

@batconjurer there's currently a gotcha in the recommendations algorithm, I think. the transfer_to_ethereum_progress RPC method skips "seen" Ethereum transfers. a transfer to be relayed is not recommended if it is found in the array of transfers returned by transfer_to_ethereum_progress; in turn, all transfers considered for relaying are obtained from querying the current signed pool.

the current signed pool may be behind the most up to date Bridge pool, because of the vote extension tx lag. so, I believe the algorithm may recommend a transfer that has already been relayed, that still shows up in the signed pool.

a simple solution may be to return with an error if the signed Bridge pool nonce does not match the up to date Bridge pool nonce in storage. this way we never recommend transfers that have already been relayed and acted upon

sug0 added a commit that referenced this pull request Aug 16, 2023
@sug0 sug0 force-pushed the tiago/fix-bp-recommendations branch from 8079c9a to 1750a8e Compare August 16, 2023 15:46
sug0 added a commit that referenced this pull request Aug 16, 2023
@sug0 sug0 force-pushed the tiago/fix-bp-recommendations branch from 1750a8e to b4b73e2 Compare August 16, 2023 15:53
sug0 added a commit that referenced this pull request Aug 16, 2023
@sug0 sug0 force-pushed the tiago/fix-bp-recommendations branch from b4b73e2 to 1274ecb Compare August 16, 2023 19:35
@sug0 sug0 marked this pull request as ready for review August 16, 2023 19:35
@sug0 sug0 requested a review from batconjurer August 16, 2023 19:35
sug0 added a commit that referenced this pull request Aug 17, 2023
@sug0 sug0 marked this pull request as draft August 28, 2023 12:59
@sug0 sug0 force-pushed the tiago/fix-bp-recommendations branch from ead7d26 to c4e4819 Compare August 28, 2023 14:44
@sug0 sug0 marked this pull request as ready for review August 28, 2023 14:45
@sug0 sug0 mentioned this pull request Aug 28, 2023
@sug0 sug0 changed the base branch from tiago/bridge-pool-fees to main August 28, 2023 14:54
@sug0 sug0 dismissed tzemanovic’s stale review August 28, 2023 14:54

The base branch was changed.

let query_validators = query_validators!();
let epoch_0_validators = query_validators(0);
let epoch_1_validators = query_validators(1);
_ = query_validators;
Copy link
Member

Choose a reason for hiding this comment

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

what is the purpose of this?

Fraccaman added a commit that referenced this pull request Sep 6, 2023
* origin/tiago/fix-bp-recommendations:
  Add changelog for #1811
  Ignore invalid conversion rates
  Add test_conversion_table_profit_margin() unit test
  Proper Debug impl for I256 values
  Return recommendation data in `generate_recommendations`
  Test the height of the BP proof stored in the db
  Check BP root's height when vote extension was signed
  Store height when BP root was signed
  Fail recommending BP batch of txs if nonce is out of date
  Prohibit generating BP proofs when the signed root is outdated
  More conversion table unit tests
  Abstract `generate_eligible` unit tests with a helper fn
  Add test_generate_eligible_happy_path() unit test
  Avoid overflowing when calculating earned gwei amount
  Improve conversion rate query
  Factor out `generate_eligible()`
  Fix Bridge pool proof warning voting power
  Rename `generate` to `generate_recommendations`
  Refactor recommendations to use EligibleRecommendation
  Add context to CLI Bridge pool cmds
  Fix Bridge pool `construct_proof`
  Convert gwei to tokens in Bridge pool conversion table
  Bridge pool conversion rates table
  Retrieve the inner raw string on wrapped context values
Fraccaman added a commit that referenced this pull request Sep 6, 2023
* origin/tiago/fix-bp-recommendations:
  Add changelog for #1811
  Ignore invalid conversion rates
  Add test_conversion_table_profit_margin() unit test
  Proper Debug impl for I256 values
  Return recommendation data in `generate_recommendations`
  Test the height of the BP proof stored in the db
  Check BP root's height when vote extension was signed
  Store height when BP root was signed
  Fail recommending BP batch of txs if nonce is out of date
  Prohibit generating BP proofs when the signed root is outdated
  More conversion table unit tests
  Abstract `generate_eligible` unit tests with a helper fn
  Add test_generate_eligible_happy_path() unit test
  Avoid overflowing when calculating earned gwei amount
  Improve conversion rate query
  Factor out `generate_eligible()`
  Fix Bridge pool proof warning voting power
  Rename `generate` to `generate_recommendations`
  Refactor recommendations to use EligibleRecommendation
  Add context to CLI Bridge pool cmds
  Fix Bridge pool `construct_proof`
  Convert gwei to tokens in Bridge pool conversion table
  Bridge pool conversion rates table
  Retrieve the inner raw string on wrapped context values
Fraccaman added a commit that referenced this pull request Sep 6, 2023
* origin/tiago/fix-bp-recommendations:
  Add changelog for #1811
  Ignore invalid conversion rates
  Add test_conversion_table_profit_margin() unit test
  Proper Debug impl for I256 values
  Return recommendation data in `generate_recommendations`
  Test the height of the BP proof stored in the db
  Check BP root's height when vote extension was signed
  Store height when BP root was signed
  Fail recommending BP batch of txs if nonce is out of date
  Prohibit generating BP proofs when the signed root is outdated
  More conversion table unit tests
  Abstract `generate_eligible` unit tests with a helper fn
  Add test_generate_eligible_happy_path() unit test
  Avoid overflowing when calculating earned gwei amount
  Improve conversion rate query
  Factor out `generate_eligible()`
  Fix Bridge pool proof warning voting power
  Rename `generate` to `generate_recommendations`
  Refactor recommendations to use EligibleRecommendation
  Add context to CLI Bridge pool cmds
  Fix Bridge pool `construct_proof`
  Convert gwei to tokens in Bridge pool conversion table
  Bridge pool conversion rates table
  Retrieve the inner raw string on wrapped context values
@Fraccaman Fraccaman merged commit 7599bde into main Sep 6, 2023
4 checks passed
@Fraccaman Fraccaman deleted the tiago/fix-bp-recommendations branch September 6, 2023 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix Bridge pool recommendations SDK functionality
4 participants