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

[ADP-3344] Add TimeInterpreter to Cardano.Wallet.Deposit.Write [old] #4816

Conversation

HeinrichApfelmus
Copy link
Contributor

@HeinrichApfelmus HeinrichApfelmus commented Oct 22, 2024

This pull request imports the TimeInterpreter type in Cardano.Wallet.Deposit.Time and adds it to the NetworkEnv.

Comments

  • This pull request is part of the preparations for using balanceTx in createPayment
  • Cardano.Wallet.Deposit.Time is a temporary location for the re-export, until we separate out the TimeInterpreter-related stuff into a separate library.

Issue Number

ADP-3344

@HeinrichApfelmus HeinrichApfelmus self-assigned this Oct 22, 2024
@HeinrichApfelmus HeinrichApfelmus changed the base branch from master to HeinrichApfelmus/ADP-3344/write-balance-tx October 22, 2024 13:13
@HeinrichApfelmus HeinrichApfelmus force-pushed the HeinrichApfelmus/ADP-3344/time-interpreter branch 2 times, most recently from 33b9b08 to 99bd214 Compare October 22, 2024 13:30
@@ -60,6 +60,7 @@ library
, cardano-strict-containers
, cardano-wallet
, cardano-wallet-network-layer
, cardano-wallet-primitive
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you avoid this pls. Do we need the Interpreter or just some functions in m ?

Copy link
Contributor Author

@HeinrichApfelmus HeinrichApfelmus Oct 22, 2024

Choose a reason for hiding this comment

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

Not happy about it either, but I want the TimeInterpreter — it's reasonably well-designed. 🤔 One of the next steps would be to split off the TimeInterpreter into a separate package and get rid of the dependency on -primitive again.

(And also remove the m parameter of the TimeInterpreter, it's more hassle than benefit.)

Copy link
Member

@Anviking Anviking Oct 22, 2024

Choose a reason for hiding this comment

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

From the perspective of createPayment or a deposit wallet specific balanceTx wrapper I suspect you might ideally want to hide both PParams and TimeTranslation inside some single LocalNodeTipStateForTxWrite. Both for the sake of hiding the details, but also for the sake of querying it atomically from the node. But the read side will need the TimeInterpreter, or similar, too.

Copy link
Member

@Anviking Anviking Oct 22, 2024

Choose a reason for hiding this comment

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

also for the sake of querying it atomically from the node

For the deposit wallet this would probably not matter (balanceTx only uses the TimeTranslation to evaluate execution units, and we don't have any UTxO or deposit lookups). In general, perhaps we still deem it not to matter, but it does lead to code like

-- Assumes the 'utxo' was queried from the node /after/ the 'era'. As
-- rolling back to a previous era should be impossible, we know 'IsRecentEra
-- era => IsRecentEra eraOfUTxO'.
forceUTxOToEra
:: Write.MaybeInRecentEra Write.UTxO
-> IO (Write.UTxO era)
forceUTxOToEra = \case
InRecentEraConway utxo -> hoist $ Write.forceUTxOToEra utxo
InRecentEraBabbage utxo -> hoist $ Write.forceUTxOToEra utxo
InNonRecentEraAlonzo -> impossibleRollback
InNonRecentEraMary -> impossibleRollback
InNonRecentEraAllegra -> impossibleRollback
InNonRecentEraShelley -> impossibleRollback
InNonRecentEraByron -> impossibleRollback
where
impossibleRollback = error "forceUTxOToEra: era should not roll back"
hoist = either (throwIO . ExceptionInvalidTxOutInEra) pure

Copy link
Collaborator

Choose a reason for hiding this comment

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

I have no problem with the m as long as it's the way to get a pure functions out of it that we can reuse during atomic operations like ingesting a block or translating time in the tx history.
But I would prefer to enumerate the exact functions we need out of it in the interface and leave the implementations of those to later. That would make mocking easier and avoid implementation leaks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed in a all, I have created a new indirection module Cardano.Wallet.Deposit.Time as a receptacle for slot-time-related stuff which we want to sort out later.

but also for the sake of querying it atomically from the node

That's a fair point. Probably not in this pull request, though.

Base automatically changed from HeinrichApfelmus/ADP-3344/write-balance-tx to master October 22, 2024 14:42
@HeinrichApfelmus HeinrichApfelmus force-pushed the HeinrichApfelmus/ADP-3344/time-interpreter branch 2 times, most recently from 6e1dbcc to 07f8a1e Compare October 23, 2024 21:50
@HeinrichApfelmus HeinrichApfelmus marked this pull request as draft October 23, 2024 21:51
@HeinrichApfelmus HeinrichApfelmus changed the base branch from master to HeinrichApfelmus/ADP-3344/network-pparams October 23, 2024 21:56
@HeinrichApfelmus HeinrichApfelmus force-pushed the HeinrichApfelmus/ADP-3344/network-pparams branch from 3853bdb to 8fcb91e Compare October 23, 2024 22:01
@HeinrichApfelmus HeinrichApfelmus force-pushed the HeinrichApfelmus/ADP-3344/time-interpreter branch from 07f8a1e to d035c0d Compare October 23, 2024 22:02
@HeinrichApfelmus HeinrichApfelmus force-pushed the HeinrichApfelmus/ADP-3344/time-interpreter branch from d035c0d to b8066f6 Compare October 24, 2024 11:11
Copy link
Collaborator

@paolino paolino left a comment

Choose a reason for hiding this comment

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

Thanks!

@HeinrichApfelmus HeinrichApfelmus marked this pull request as ready for review October 24, 2024 12:45
@HeinrichApfelmus HeinrichApfelmus merged commit c7f5522 into HeinrichApfelmus/ADP-3344/network-pparams Oct 24, 2024
25 checks passed
@HeinrichApfelmus HeinrichApfelmus deleted the HeinrichApfelmus/ADP-3344/time-interpreter branch October 24, 2024 12:46
@HeinrichApfelmus HeinrichApfelmus restored the HeinrichApfelmus/ADP-3344/time-interpreter branch October 24, 2024 12:48
@HeinrichApfelmus HeinrichApfelmus changed the title [ADP-3344] Add TimeInterpreter to Cardano.Wallet.Deposit.Write [ADP-3344] Add TimeInterpreter to Cardano.Wallet.Deposit.Write [old] Oct 24, 2024
github-merge-queue bot pushed a commit that referenced this pull request Oct 24, 2024
)

This pull request imports the `TimeInterpreter` type in
`Cardano.Wallet.Deposit.Time` and adds it to the `NetworkEnv`.

### Comments

* This pull request is part of the preparations for using `balanceTx` in
`createPayment`
* `Cardano.Wallet.Deposit.Time` is a temporary location for the
re-export, until we separate out the `TimeInterpreter`-related stuff
into a separate library.
* I messed up merging of #4816, this is the exact equal pull request,
but with `master` as target.

### Issue Number

ADP-3344
@HeinrichApfelmus HeinrichApfelmus deleted the HeinrichApfelmus/ADP-3344/time-interpreter branch October 24, 2024 14:20
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.

3 participants