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

Add CIP-66 receipt field feeInFeeCurrency #2292

Draft
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

hbandura
Copy link
Contributor

@hbandura hbandura commented Apr 8, 2024

Add the receipt field for CELO denominated txs
Add e2etest for CELO denominated txs
Fix minor bugs in tx JSON marshalling
Remove e2e tests pertaining previous fork activation

Copy link

github-actions bot commented Apr 8, 2024

5882 passed, 45 skipped

Copy link

github-actions bot commented Apr 8, 2024

Coverage from tests in ./e2e_test/... for ./consensus/istanbul/... at commit 51f8f08

coverage: 49.7% of statements across all listed packages
coverage:  63.4% of statements in consensus/istanbul
coverage:  42.9% of statements in consensus/istanbul/announce
coverage:  54.8% of statements in consensus/istanbul/backend
coverage:   0.0% of statements in consensus/istanbul/backend/backendtest
coverage:  24.3% of statements in consensus/istanbul/backend/internal/replica
coverage:  62.1% of statements in consensus/istanbul/core
coverage:  50.0% of statements in consensus/istanbul/db
coverage:   0.0% of statements in consensus/istanbul/proxy
coverage:  64.2% of statements in consensus/istanbul/uptime
coverage:  51.8% of statements in consensus/istanbul/validator
coverage:  79.2% of statements in consensus/istanbul/validator/random

e2e_test/e2e_transfer_test.go Show resolved Hide resolved
FeeInFeeCurrency *big.Int
}

type preCIP66receiptRLP struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should keep the old structures untouched for easier maintenance and use a specifically-typed structure for the new TX type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The receipt types are a bit confusing, technically here the old ones are untouched, and this one is only used for CELO denominated txs, as seen in the Receipts encode index (and decode as well).

But it's true that it feels like a weird change from this perspective

@hbandura hbandura marked this pull request as draft April 18, 2024 06:35
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.

2 participants