Move slotting to separate module #1890
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Anviking
force-pushed
the
anviking/1868/move-slotting
branch
from
62b4720 to
1d1bea8
Compare
Anviking
force-pushed
the
anviking/1868/rename-slotNo
branch
from
48d3ccd to
8a85564
Compare
Anviking
force-pushed
the
anviking/1868/move-slotting
branch
from
1d1bea8 to
c238d9b
Compare
Anviking
commented
Jul 9, 2020
KtorZ
force-pushed
the
anviking/1868/rename-slotNo
branch
from
8a85564 to
357bb22
Compare
Anviking
force-pushed
the
anviking/1868/move-slotting
branch
from
df09036 to
b2e34a7
Compare
5 tasks
rvl
approved these changes
Jul 14, 2020
Comment on lines
+67
to
+72
| import Ouroboros.Consensus.HardFork.History.EraParams | ||
| ( EraParams (..), noLowerBoundSafeZone ) | ||
| import Ouroboros.Consensus.HardFork.History.Qry | ||
| ( Qry, runQuery, slotToEpoch ) | ||
| import Ouroboros.Consensus.HardFork.History.Summary | ||
| ( Summary (..), neverForksSummary ) |
There was a problem hiding this comment.
Importing of Ouroboros.Consensus doesn't seem right for the cardano-wallet-core package - it is cardano-node backend specific. Is there any way to move this to the cardano-wallet-shelley package?
There was a problem hiding this comment.
Core already depends on ouroboros-network: https://github.com/input-output-hk/cardano-wallet/blob/315b875cd3c6a78b8c4d4e58bfec6bd4d9acffcd/lib/core/src/Ouroboros/Network/Client/Wallet.hs#L12-L20
- To avoid having
Qryin core, we'd need to duplicate the interface - Jormungandr can satisfy the
Qryinterface
I do think it is fine.
Slightly related thread: point vs forcing jormungandr to use SlotNo
There was a problem hiding this comment.
The boundary between jormungandr and byron/shelley could also be revisited later.
Comment on lines
+89
to
+90
| -- TODO: The type should be changed to @Interpreter@ when we bump | ||
| -- ouroboros-consensus. |
There was a problem hiding this comment.
Should we do this? Another PR?
There was a problem hiding this comment.
No rush. I don't think we need it until #1870 is starting.
- We will use it to correctly deal with time across hard-forks - Added tests for equivalence with old implementation
Anviking
force-pushed
the
anviking/1868/move-slotting
branch
from
b2e34a7 to
1be1ac1
Compare
|
bors r+ |
Build succeeded |
iohk-bors bot
added a commit
that referenced
this pull request
Jul 17, 2020
1901: Use SlotNo and TimeInterpreter r=Anviking a=Anviking # Issue Number ADP-356 #1868 #1869 # Overview - [x] Add TimeInterpreter, necessary queries, and a way to run them Add tests comparing them with old implementation - [x] Fundamentally change wallet to use SlotNo instead of SlotId - Patch together cardano-wallet-jormungandr to still work, despite the binary using SlotId. - [x] Remove redundant byron code - [x] Sync progress calculation compares times now instead of slots - [x] Rename the `slot` tables to `slot_no` to force automatic migration, preventing catastrophic re-interpretation of old data # Comments - Depends on #1890 <!-- Additional comments or screenshots to attach if any --> <!-- Don't forget to: ✓ Self-review your changes to make sure nothing unexpected slipped through ✓ Assign yourself to the PR ✓ Assign one or several reviewer(s) ✓ Once created, link this PR to its corresponding ticket ✓ Assign the PR to a corresponding milestone ✓ Acknowledge any changes required to the Wiki --> Co-authored-by: Johannes Lund <[email protected]>
iohk-bors bot
added a commit
that referenced
this pull request
Jul 17, 2020
1901: Use SlotNo and TimeInterpreter r=Anviking a=Anviking # Issue Number ADP-356 #1868 #1869 # Overview - [x] Add TimeInterpreter, necessary queries, and a way to run them Add tests comparing them with old implementation - [x] Fundamentally change wallet to use SlotNo instead of SlotId - Patch together cardano-wallet-jormungandr to still work, despite the binary using SlotId. - [x] Remove redundant byron code - [x] Sync progress calculation compares times now instead of slots - [x] Rename the `slot` tables to `slot_no` to force automatic migration, preventing catastrophic re-interpretation of old data # Comments - Depends on #1890 <!-- Additional comments or screenshots to attach if any --> <!-- Don't forget to: ✓ Self-review your changes to make sure nothing unexpected slipped through ✓ Assign yourself to the PR ✓ Assign one or several reviewer(s) ✓ Once created, link this PR to its corresponding ticket ✓ Assign the PR to a corresponding milestone ✓ Acknowledge any changes required to the Wiki --> Co-authored-by: Johannes Lund <[email protected]>
iohk-bors bot
added a commit
that referenced
this pull request
Jul 17, 2020
1901: Use SlotNo and TimeInterpreter r=Anviking a=Anviking # Issue Number ADP-356 #1868 #1869 # Overview - [x] Add TimeInterpreter, necessary queries, and a way to run them Add tests comparing them with old implementation - [x] Fundamentally change wallet to use SlotNo instead of SlotId - Patch together cardano-wallet-jormungandr to still work, despite the binary using SlotId. - [x] Remove redundant byron code - [x] Sync progress calculation compares times now instead of slots - [x] Rename the `slot` tables to `slot_no` to force automatic migration, preventing catastrophic re-interpretation of old data # Comments - Depends on #1890 <!-- Additional comments or screenshots to attach if any --> <!-- Don't forget to: ✓ Self-review your changes to make sure nothing unexpected slipped through ✓ Assign yourself to the PR ✓ Assign one or several reviewer(s) ✓ Once created, link this PR to its corresponding ticket ✓ Assign the PR to a corresponding milestone ✓ Acknowledge any changes required to the Wiki --> Co-authored-by: Johannes Lund <[email protected]>
iohk-bors bot
added a commit
that referenced
this pull request
Jul 17, 2020
1901: Use SlotNo and TimeInterpreter r=Anviking a=Anviking # Issue Number ADP-356 #1868 #1869 # Overview - [x] Add TimeInterpreter, necessary queries, and a way to run them Add tests comparing them with old implementation - [x] Fundamentally change wallet to use SlotNo instead of SlotId - Patch together cardano-wallet-jormungandr to still work, despite the binary using SlotId. - [x] Remove redundant byron code - [x] Sync progress calculation compares times now instead of slots - [x] Rename the `slot` tables to `slot_no` to force automatic migration, preventing catastrophic re-interpretation of old data # Comments - Depends on #1890 <!-- Additional comments or screenshots to attach if any --> <!-- Don't forget to: ✓ Self-review your changes to make sure nothing unexpected slipped through ✓ Assign yourself to the PR ✓ Assign one or several reviewer(s) ✓ Once created, link this PR to its corresponding ticket ✓ Assign the PR to a corresponding milestone ✓ Acknowledge any changes required to the Wiki --> Co-authored-by: Johannes Lund <[email protected]>
iohk-bors bot
added a commit
that referenced
this pull request
Jul 17, 2020
1901: Use SlotNo and TimeInterpreter r=Anviking a=Anviking # Issue Number ADP-356 #1868 #1869 # Overview - [x] Add TimeInterpreter, necessary queries, and a way to run them Add tests comparing them with old implementation - [x] Fundamentally change wallet to use SlotNo instead of SlotId - Patch together cardano-wallet-jormungandr to still work, despite the binary using SlotId. - [x] Remove redundant byron code - [x] Sync progress calculation compares times now instead of slots - [x] Rename the `slot` tables to `slot_no` to force automatic migration, preventing catastrophic re-interpretation of old data # Comments - Depends on #1890 <!-- Additional comments or screenshots to attach if any --> <!-- Don't forget to: ✓ Self-review your changes to make sure nothing unexpected slipped through ✓ Assign yourself to the PR ✓ Assign one or several reviewer(s) ✓ Once created, link this PR to its corresponding ticket ✓ Assign the PR to a corresponding milestone ✓ Acknowledge any changes required to the Wiki --> Co-authored-by: Johannes Lund <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue Number
ADP-356 #1868, #1869
Overview
flatSlot,slotAt, etc.) to a new moduleCardano.Wallet.Primitive.SlottingrunQuery epochOf singleEraInterpreter == epochNumber . fromFlatSlotComments
Rangetests and slotting tests seemed unclear.Qry/Interpreter.