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

Create cardano-node-emulator #831

Merged
merged 2 commits into from Dec 16, 2022
Merged

Create cardano-node-emulator #831

merged 2 commits into from Dec 16, 2022

Conversation

ak3n
Copy link
Contributor

@ak3n ak3n commented Nov 22, 2022 •
edited

This PR implements the ADR from #835:

  • bootstraps cardano-node-emulator cabal package;
  • moves:
    • Ledger.TimeSlot to Cardano.Node.Emulator.TimeSlot
    • Ledger.Params to Cardano.Node.Emulator.Params
    • Ledger.Generators to Cardano.Node.Emulator.Generators
    • Ledger.Fee to Cardano.Node.Emulator.Fee
    • Ledger.Validation to Cardano.Node.Emulator.Validation
    • Wallet.Emulator.Chain to Cardano.Node.Emulator.Chain

Pre-submit checklist:

  • Branch
    • Tests are provided (if possible)
    • Commit sequence broadly makes sense
    • Key commits have useful messages
    • Formatting, PNG optimization, etc. are updated
    • Important changes are reflected in changelog.d of the affected packages
  • PR
    • Self-reviewed the diff
    • Useful pull request description
    • Reference the ADR in the PR and reference the PR in the ADR (if revelant)
    • Reviewer requested

@koslambrou koslambrou mentioned this pull request Nov 24, 2022
Merged
9 tasks
@ak3n ak3n force-pushed the split-plutus-contract branch 2 times, most recently from 9e707c0 to 33dc4a6 Compare November 30, 2022 10:14
@ak3n ak3n marked this pull request as ready for review December 5, 2022 11:44
ak3n
ak3n commented Dec 5, 2022
View reviewed changes
cardano-node-emulator/src/Cardano/Node/Emulator/Chain.hs
Comment on lines -67 to +69
_currentSlot :: Slot -- ^ The current slot number
_chainCurrentSlot :: Slot -- ^ The current slot number
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had to rename it because of the conflicting export. Also like it better this way.

plutus-ledger/src/Ledger/Tx/CardanoAPI.hs
Comment on lines -52 to +70
toCardanoTxBodyContent p@P.Params{P.pNetworkId} sigs tx@P.Tx{..} = do
toCardanoTxBodyContent networkId protocolParams sigs tx@P.Tx{..} = do
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since cardano-node-emulator depends on plutus-ledger for now, to avoid the circular dependency issue I decided to drop Params and other types and inline them directly.

When we will figure the better design of plutus-ledger, we will continue to use the normal types.

@sjoerdvisscher
Copy link
Contributor

How would you describe the things you left in plutus-ledger?

koslambrou
koslambrou approved these changes Dec 6, 2022
View reviewed changes
cardano-node-emulator/src/Cardano/Node/Emulator/Effects.hs Outdated
import Control.Monad.Freer.TH (makeEffect)
import Ledger (CardanoTx, Slot)

data NodeClientEffect r where
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm beginning to think that this should stay in plutus-contract...

This effect seems to be for supporting multiple node backends in the emulator. So by interpreting this effect differently, you can use a real node, or the API provided by cardano-node-emulator.

In the future, if cardano-node-emulator uses the same IPC interface as cardano-node, then this effect would not be necessary anymore.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree! Makes sense.

catch-21 referenced this pull request Dec 6, 2022
Add ADR doc for end-to-end testing strategy
b420133
@ak3n
Copy link
Contributor Author

ak3n commented Dec 8, 2022

@sjoerdvisscher we discussed that with @koslambrou. plutus-ledger and is more like plutus-core or kind of base package with many small things which should be distributed among proper packages.

berewt
berewt approved these changes Dec 8, 2022
View reviewed changes
Copy link
Contributor

@berewt berewt left a comment

Choose a reason for hiding this comment

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

LGTM

@ak3n ak3n force-pushed the split-plutus-contract branch 7 times, most recently from 13211c4 to 4733ad7 Compare December 16, 2022 11:44
@ak3n
Create cardano-node-emulator package
1ae4590 
Move Params and TimeSlot modules to cardano-node-emulator

Move some functions from Validation to CardanoAPI

Add changelog files

Fix formatting
@koslambrou koslambrou merged commit 8706e6c into main Dec 16, 2022
@koslambrou koslambrou deleted the split-plutus-contract branch December 16, 2022 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sjoerdvisscher sjoerdvisscher sjoerdvisscher approved these changes

@berewt berewt berewt approved these changes

@koslambrou koslambrou koslambrou approved these changes

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

Successfully merging this pull request may close these issues.

None yet
4 participants
@ak3n @sjoerdvisscher @berewt @koslambrou