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.

Sign up for GitHub

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

Jump to bottom

Add helpers for txpool #27

Closed
wants to merge 1 commit into from
Closed

Add helpers for txpool #27

wants to merge 1 commit into from

Conversation

palango
Copy link

@palango palango commented Oct 26, 2023 •
edited
Loading

Resolves #72

Adds a new CeloContext as described in #72. Will need changes after #26 .

Open questions

  • What should we do if a fee currency is whitelisted but we cannot get an exchange rate?
  • Can we use the exchange rates available in the BlockContext?

@palango palango requested a review from hbandura October 26, 2023 15:11
@karlb
Copy link

karlb commented Oct 27, 2023

What should we do if a fee currency is whitelisted but we cannot get an exchange rate?

Is there any real choice but not to include txs with that fee currency?

@palango
Copy link
Author

palango commented Oct 27, 2023 •
edited
Loading

What should we do if a fee currency is whitelisted but we cannot get an exchange rate?

Is there any real choice but not to include txs with that fee currency?

I'm asking because this is not what we currently do: https://github.com/celo-org/celo-blockchain/blob/d2e7c9f3a44a727a8117da9c928a3ccb8e582f27/contracts/currency/currency.go#L206

But this behaviour doesn't make sense to me either and I think we should avoid those transactions.

@hbandura
Copy link

Maybe the "IsWhitelisted" should be something like "IsAvailable", and if we cannot find the exchange rate OR if it's not whitelisted, we should drop all txs

@palango
Copy link
Author

palango commented Oct 31, 2023

Maybe the "IsWhitelisted" should be something like "IsAvailable", and if we cannot find the exchange rate OR if it's not whitelisted, we should drop all txs

It behaves like that now. But I still think IsWhitelisted fit's better.

carterqw2
carterqw2 reviewed Nov 2, 2023
View reviewed changes
core/txpool/legacypool/celo.go Outdated Show resolved Hide resolved
core/celo_evm.go Outdated Show resolved Hide resolved
core/txpool/legacypool/celo_test.go Outdated Show resolved Hide resolved
core/txpool/legacypool/celo.go Outdated Show resolved Hide resolved
@palango
Add CeloContext
c7f30a9 
Add tests

Rebase

tmp

Add error

PR review

CHanges after discussion
@palango palango changed the title Add CeloContext for txpool Add helpers for txpool Nov 7, 2023
@palango palango mentioned this pull request Nov 30, 2023
Closed
@carterqw2 carterqw2 mentioned this pull request Nov 30, 2023
Closed
@palango
Copy link
Author

palango commented Feb 19, 2024

Included in #43

@palango palango closed this Feb 19, 2024
@palango palango deleted the palango/celoctx branch June 12, 2024 08:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carterqw2 carterqw2 carterqw2 left review comments

@hbandura hbandura Awaiting requested review from hbandura

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add helpers for fee currency handling
4 participants
@palango @karlb @hbandura @carterqw2