Handle the hash calculation DirectCall change to maintain backwards compatibility
#339
Conversation
WalkthroughThe changes manage the alteration in the hash calculation method for Changes
Sequence DiagramsDirectCall Hash Calculation ChangesequenceDiagram
participant User
participant System
User ->> System: Start loading transaction
System ->> Config: Check HashCalculationHeightChange
Config -->> System: Return Height
System ->> Transaction: Fetch data with block height
Transaction ->> DirectCall: Initialize with block height
DirectCall ->> DirectCall: Conditional hash calculation
DirectCall -->> Transaction: Return hash
Transaction -->> System: Return processed transaction
System -->> User: Display processed transaction
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- bootstrap/bootstrap.go (1 hunks)
- config/config.go (2 hunks)
- models/transaction.go (2 hunks)
- models/transaction_test.go (3 hunks)
- storage/pebble/transactions.go (2 hunks)
- tests/helpers.go (1 hunks)
Additional comments not posted (14)
storage/pebble/transactions.go (3)
4-4: Import statement added.The
encoding/binarypackage is imported to handle byte order conversions, which is necessary for the new functionality.
50-56: Ensure error handling for height retrieval.The
Getmethod now retrievescadenceHeight. Ensure that the fallback to0is appropriate and does not mask potential issues.
57-57: PasscadenceHeighttoUnmarshalTransaction.The
cadenceHeightis correctly passed tomodels.UnmarshalTransactionto handle the new hash calculation logic.models/transaction.go (4)
56-57: AddblockHeightfield toDirectCall.The
blockHeightfield is added to handle the conditional hash calculation. Ensure this temporary field is removed after the PreviewNet reset.
60-61: Temporary configuration parameter added.The
DirectCallHashCalculationBlockHeightChangeparameter is introduced to handle the hash calculation change. Ensure this is removed after the PreviewNet reset.
64-76: Conditional hash calculation inHashmethod.The
Hashmethod now conditionally calculates the hash based onblockHeight. This change ensures backward compatibility.
215-223: UpdateUnmarshalTransactionto acceptblockHeight.The
UnmarshalTransactionmethod now acceptsblockHeightand initializesDirectCallwith it. This change supports the new hash calculation logic.bootstrap/bootstrap.go (1)
97-99: Temporary assignment of configuration parameter.The
DirectCallHashCalculationBlockHeightChangeis assigned fromcfg.HashCalculationHeightChange. Ensure this is removed after the PreviewNet reset.tests/helpers.go (1)
157-157: AddHashCalculationHeightChangeto test configuration.The
HashCalculationHeightChangefield is added to theConfigstruct in theservicesSetupfunction to support testing the new hash calculation logic.config/config.go (2)
94-95: LGTM! New configuration parameter added.The
HashCalculationHeightChangefield has been correctly added to theConfigstruct.
155-157: LGTM! New flag added.The
hash-calc-height-changeflag has been correctly added to theFromFlagsfunction to set theHashCalculationHeightChangevalue.models/transaction_test.go (3)
Line range hint
190-194:
LGTM! Test case updated.The test case
with TransactionCall valuehas been correctly updated to include theDirectCallHashCalculationBlockHeightChangeparameter in theUnmarshalTransactionfunction call.
Line range hint
244-248:
LGTM! Test case updated.The test case
with DirectCall valuehas been correctly updated to include theDirectCallHashCalculationBlockHeightChangeparameter in theUnmarshalTransactionfunction call.
283-341: LGTM! New test case added.The new test case
with DirectCall hash calculation changecorrectly tests the hash calculation changes based on block height. It verifies both the new and old hash calculations.
m-Peter
force-pushed
the
handle-direct-call-hash-calc-change
branch
from
807151c to
0271686
Compare
| return models.UnmarshalTransaction(val) | ||
| heightVal, err := t.store.get(latestCadenceHeightKey) | ||
| if err != nil { | ||
| heightVal = []byte{0, 0, 0, 0, 0, 0, 0, 0} |
There was a problem hiding this comment.
In case of err, I am just adding a default value of 0. Not sure how to handle that. I am open to ideas.
m-Peter
force-pushed
the
handle-direct-call-hash-calc-change
branch
from
0271686 to
0b75260
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
bootstrap/bootstrap.go (1)
97-99: Add a TODO comment for tracking temporary code.The comment indicates that this assignment is temporary and should be removed after PreviewNet is reset. To ensure this is tracked, consider adding a TODO comment.
// TEMP: Remove `DirectCallHashCalculationBlockHeightChange` after PreviewNet is reset + // TODO: Remove this temporary assignment after PreviewNet reset models.DirectCallHashCalculationBlockHeightChange = cfg.HashCalculationHeightChange
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- bootstrap/bootstrap.go (1 hunks)
- config/config.go (2 hunks)
- models/transaction.go (2 hunks)
- models/transaction_test.go (3 hunks)
- storage/pebble/transactions.go (2 hunks)
- tests/helpers.go (1 hunks)
Files skipped from review as they are similar to previous changes (5)
- config/config.go
- models/transaction.go
- models/transaction_test.go
- storage/pebble/transactions.go
- tests/helpers.go
Closes: #338
Description
Add a temporary config flag for setting the Cadence height at which we should be using the new hash calculation.
For previous transactions, we need to use the old hash calculation.
This will unblock deployments of new features on PreviewNet.
For contributor use:
masterbranchFiles changedin the Github PR explorerSummary by CodeRabbit
New Features
Bug Fixes
Tests
Chores