Skip to content
This repository was archived by the owner on Dec 2, 2024. It is now read-only.

PLT-777 Remove plutus ledger from Marconi' dependencies#792

Merged
berewt merged 10 commits intomainfrom
nicolas/PLT-777
Nov 3, 2022
Merged

PLT-777 Remove plutus ledger from Marconi' dependencies#792
berewt merged 10 commits intomainfrom
nicolas/PLT-777

Conversation

@berewt
Copy link
Contributor

@berewt berewt commented Oct 27, 2022 •
edited
Loading

Here are the major type changes:

  • TxOutRef (from plutus-ledger-api) has been replaced by an alias with the same name to TxIn of cardano-api that conveys the same information.
  • Address (from plutus-ledger) has been replaced by AddressAny from cardano-api, that is basically an address in era, without the era witness type.
  • TxOut (from plutus-ledger) has been replaced by TxOut from cardana-api.
  • Datum and DatumHash (from plutus-ledger-api) has been replaced by ScriptData and Hash ScriptData from cardano-api.

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
  • 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

@berewt berewt marked this pull request as ready for review October 27, 2022 06:46
@berewt berewt requested review from eyeinsky, kayvank and raduom October 27, 2022 06:46
@berewt berewt marked this pull request as draft October 27, 2022 08:08
@berewt berewt marked this pull request as ready for review October 27, 2022 13:05
@berewt
Copy link
Contributor Author

berewt commented Oct 27, 2022 •
edited
Loading

I'm gonna make a PR to cardano-node to expose the function I had to duplicate and fix the corresponding comment in my code once before merging it.

@koslambrou
Copy link
Contributor

koslambrou commented Nov 2, 2022

You're waiting for the node PR to merge before merging this PR?

@berewt
Copy link
Contributor Author

berewt commented Nov 2, 2022

You're waiting for the node PR to merge before merging this PR?

I think we'd better merge it before the node PR, just to avoid adding new features that rely on plutus-ledger in the meantime.

kayvank
kayvank approved these changes Nov 3, 2022
View reviewed changes
Copy link
Collaborator

@kayvank kayvank left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thank you for doing the work. I specially like ❤️ the Marconi.Types and plan on moving some of data structure to there from the Indexers.hs
FYI, this work directlry effect PR-796. I'll merge mine after this PR is merged to main

@berewt
Copy link
Contributor Author

berewt commented Nov 3, 2022

I'll merge mine after this PR is merged to main

If yours is ready to merge, don't worry and merge it, I'll handle the conflict if needed.

raduom
raduom approved these changes Nov 3, 2022
View reviewed changes
Copy link
Contributor

@raduom raduom left a comment

Choose a reason for hiding this comment

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

LGTM

eyeinsky
eyeinsky reviewed Nov 3, 2022
View reviewed changes
eyeinsky
eyeinsky reviewed Nov 3, 2022
View reviewed changes
eyeinsky
eyeinsky approved these changes Nov 3, 2022
View reviewed changes
Copy link
Contributor

@eyeinsky eyeinsky left a comment

Choose a reason for hiding this comment

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

LGTM!

@berewt berewt merged commit 6be2ce2 into main Nov 3, 2022
@berewt berewt deleted the nicolas/PLT-777 branch November 3, 2022 15:53
koslambrou pushed a commit that referenced this pull request Apr 6, 2023
@berewt
PLT-777 Remove plutus ledger from Marconi' dependencies (#792)
faf272b 
* Use Cardano addresses instead of Plutus.Ledger addresses

* Use TxIn instead of TxOutRef

* Harmonize addresses

* A PR to cardano-api is made and the comment on the duplicated marconi function is modified accordingly

* Rename Marconi.CardanoAPI in Marconi.Types
  Centralise TargetAddresses alias here

* Update the sample invocation of marconi-mamba README
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

3 more reviewers

@kayvank kayvank kayvank approved these changes

@raduom raduom raduom approved these changes

@eyeinsky eyeinsky eyeinsky approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@berewt @koslambrou @kayvank @raduom @eyeinsky