Basic Alonzo support #2763
Merged
Basic Alonzo support #2763
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
commented
Jul 15, 2021
| @@ -635,42 +683,49 @@ fromShelleyPParams eraInfo pp = W.ProtocolParameters | |||
| -- convert it into a percentage. | |||
| -- | |||
| decentralizationLevelFromPParams | |||
| :: SLAPI.PParams era | |||
| :: HasField "_d" pparams SL.UnitInterval | |||
There was a problem hiding this comment.
If we were to use Cardano.Api we wouldn't need to do all of this juggling. But looking into it, it seems a bit all-or-nothing — we can't start using it for a single query only. Unless we ask them to expose a bunch of internal stuff.
Anviking
force-pushed
the
anviking/ADP-952/alonzo
branch
2 times, most recently
from
4312c80 to
660812f
Compare
Anviking
force-pushed
the
anviking/ADP-952/alonzo
branch
from
cfda01c to
a5389d6
Compare
Anviking
commented
Jul 22, 2021
Anviking
commented
Jul 22, 2021
|
bors try |
tryBuild failed: |
|
bors try |
tryBuild failed: |
Anviking
force-pushed
the
anviking/ADP-952/alonzo
branch
2 times, most recently
from
d7c9db3 to
5108e25
Compare
|
bors try |
tryBuild failed: |
|
Not sure why there are Info-logs in the CI integration test output… 🤔 I don't see this locally. |
|
bors try |
tryBuild succeeded: |
|
bors try |
tryBuild failed: |
|
bors try |
tryBuild failed: |
Anviking
force-pushed
the
anviking/ADP-952/alonzo
branch
from
12d003f to
1ee610c
Compare
|
I made a ticket for the weeder failures: #2785. |
Anviking
force-pushed
the
anviking/ADP-952/alonzo
branch
from
736d2cd to
55ec2a6
Compare
2 tasks
|
bors try |
rvl
approved these changes
Jul 30, 2021
There was a problem hiding this comment.
Great. There is just an outstanding todo item hese:
rodney@tethys:~/iohk/cw/alonzo % ./scripts/todo-list.sh lib -t ADP-952
+++ TODO list
267 source files [****************] 100%
lib/shelley/src/Cardano/Wallet/Shelley/Transaction.hs
1273 We presumably need to provide the protocol params if our tx uses
scripts? [ADP-952]| @@ -1,7 +1,7 @@ | |||
| # Taken from https://hydra.iohk.io/build/6782523/download/1/index.html | |||
| # and not sanity-checked for appropriateness for when we turn on Alonzo | |||
| # in our test cluster. | |||
| adaPerUTxOWord: 0 | |||
| adaPerUTxOWord: 34482 | |||
There was a problem hiding this comment.
Does the comment above still apply?
There was a problem hiding this comment.
Ah, forgot about it! Thanks, will remove.
There was a problem hiding this comment.
214bc4b Plutus script parameters probably need to be sanity checked at some point, so kept a comment about that.
Comment on lines
+2530
to
+2531
| let txConstraints = (view #constraints tl) pp | ||
| liftIO $ toApiNetworkParameters np txConstraints (interpretQuery ti . toApiEpochInfo) |
There was a problem hiding this comment.
Can we not involve TransactionLayer here?
It seems a bit roundabout to call a transaction layer thing that just references the protocol parameters.
There was a problem hiding this comment.
It seems a bit roundabout to call a transaction layer thing that just references the protocol parameters.
I fully agree. But to convert the Alonzo adaPerUTxOWord to our generic API minUTxOValue, we do need to call the cardano-wallet-specific (i.e not in -core) function computeMinimumAdaQuantityInternal…
Alternatives could be:
- Pass in using some different record/function than the TransactionLayer — but we already have this one
- Convert
MinUTxOValuetoCoinalready before storing it in theProtocolParametersrecord — sounds like a horrible idea - Merge
cardano-walletandcardano-wallet-core
Aside from the merging the libraries, I don't see anything obvious.
There was a problem hiding this comment.
Oh, that's ... unfortunate. I have opened PR #2787 to fix this.
Anviking
force-pushed
the
anviking/ADP-952/alonzo
branch
from
6c40b62 to
214bc4b
Compare
tryBuild failed: |
- Set TestEnableDevelopmentNetworkProtocols - Tweak alonzo genesis - Fix SHELLEY_CREATE_MIGRATION_PLAN_04 to work in both Mary and Alonzo
Instead of just running on the latest era (Alonzo), this makes them also run on Mary.
Since we run tests on two different eras now, we want buildkite to upload artefacts from both eras. Hopefully this should do it.
Anviking
force-pushed
the
anviking/ADP-952/alonzo
branch
from
9f1606a to
bf46b04
Compare
|
bors r+ |
iohk-bors bot
added a commit
that referenced
this pull request
Jul 30, 2021
2763: Basic Alonzo support r=Anviking a=Anviking # Issue Number ADP-1025 # Overview <!-- Detail in a few bullet points the work accomplished in this PR --> - [x] Ensure the wallet can run in Alonzo - [x] Get all integration tests green - [x] Fix warnings - [x] Cleanup - <s>[ ]Test that addresses don't affect the min ada value</s> Existing integration tests should be enough - [x] <s>Does it still work on testnet? -> The ledger function in question cannot know the current network, so would be highly surprising if it didn't work on testnet </s> - [x] Update MinUTxOValue properties to generate both ShelleyMA and Alonzo cases - [ ] Go through everything from start to finish - [x] Run integration tests on both Mary and alonzo - [x] Check that the Mary era is actually picked up, and it's not just running Alonzo twice - <s>[ ] Using cardano-node 1.28.0 tag</s> -> #2778 - <s>[x] Cabal build also works</s> -> #2776 # Comments - [Hydra jobset](https://hydra.iohk.io/jobset/Cardano/cardano-wallet-pr-2763) - [Cabal Nightly build](https://buildkite.com/input-output-hk/cardano-wallet-nightly/builds?branch=anviking%2FADP-952%2Falonzo) (use environment variable `step=cabal` when creating a new build) Co-authored-by: Johannes Lund <[email protected]> Co-authored-by: Rodney Lorrimar <[email protected]>
|
Build failed: |
Anviking
force-pushed
the
anviking/ADP-952/alonzo
branch
from
c24bac0 to
83dab6f
Compare
|
Great. I will merge without bors because it was just a weeder false positive which failed. |
WilliamKingNoel-Bot
pushed a commit
that referenced
this pull request
Jul 30, 2021
2 tasks
iohk-bors bot
added a commit
that referenced
this pull request
Aug 2, 2021
2791: Negotiate NodeToClientVersion for non-breaking mainnet support r=Anviking a=Anviking # Issue Number ADP-1025 # Background #2763 made the wallet demand NodeToClientV_9. With cardano-node 1.28.0, V_9 is only enabled if `TestEnableDevelopmentNetworkProtocols` is set in the node config. # Overview - [x] Make the wallet support both `V_8` and `V_9`. - [x] Ensure integration tests on Mary are run without `TestEnableDevelopmentNetworkProtocols: True` # Comments <!-- 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) ✓ Jira will detect and link to this PR once created, but you can also link this PR in the description of the corresponding ticket ✓ Acknowledge any changes required to the Wiki ✓ Finally, in the PR description delete any empty sections and all text commented in <!--, so that this text does not appear in merge commit messages. --> Co-authored-by: Johannes Lund <[email protected]>
iohk-bors bot
added a commit
that referenced
this pull request
Aug 2, 2021
2791: Negotiate NodeToClientVersion for non-breaking mainnet support r=Anviking a=Anviking # Issue Number ADP-1025 # Background #2763 made the wallet demand NodeToClientV_9. With cardano-node 1.28.0, V_9 is only enabled if `TestEnableDevelopmentNetworkProtocols` is set in the node config. # Overview - [x] Make the wallet support both `V_8` and `V_9`. - [x] Ensure integration tests on Mary are run without `TestEnableDevelopmentNetworkProtocols: True` # Comments <!-- 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) ✓ Jira will detect and link to this PR once created, but you can also link this PR in the description of the corresponding ticket ✓ Acknowledge any changes required to the Wiki ✓ Finally, in the PR description delete any empty sections and all text commented in <!--, so that this text does not appear in merge commit messages. --> Co-authored-by: Johannes Lund <[email protected]>
iohk-bors bot
added a commit
that referenced
this pull request
Aug 2, 2021
2791: Negotiate NodeToClientVersion for non-breaking mainnet support r=Anviking a=Anviking # Issue Number ADP-1025 # Background #2763 made the wallet demand NodeToClientV_9. With cardano-node 1.28.0, V_9 is only enabled if `TestEnableDevelopmentNetworkProtocols` is set in the node config. # Overview - [x] Make the wallet support both `V_8` and `V_9`. - [x] Ensure integration tests on Mary are run without `TestEnableDevelopmentNetworkProtocols: True` # Comments <!-- 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) ✓ Jira will detect and link to this PR once created, but you can also link this PR in the description of the corresponding ticket ✓ Acknowledge any changes required to the Wiki ✓ Finally, in the PR description delete any empty sections and all text commented in <!--, so that this text does not appear in merge commit messages. --> Co-authored-by: Johannes Lund <[email protected]>
iohk-bors bot
added a commit
that referenced
this pull request
Aug 2, 2021
2791: Negotiate NodeToClientVersion for non-breaking mainnet support r=Anviking a=Anviking # Issue Number ADP-1025 # Background #2763 made the wallet demand NodeToClientV_9. With cardano-node 1.28.0, V_9 is only enabled if `TestEnableDevelopmentNetworkProtocols` is set in the node config. # Overview - [x] Make the wallet support both `V_8` and `V_9`. - [x] Ensure integration tests on Mary are run without `TestEnableDevelopmentNetworkProtocols: True` # Comments <!-- 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) ✓ Jira will detect and link to this PR once created, but you can also link this PR in the description of the corresponding ticket ✓ Acknowledge any changes required to the Wiki ✓ Finally, in the PR description delete any empty sections and all text commented in <!--, so that this text does not appear in merge commit messages. --> Co-authored-by: Johannes Lund <[email protected]>
iohk-bors bot
added a commit
that referenced
this pull request
Aug 2, 2021
2791: Negotiate NodeToClientVersion for non-breaking mainnet support r=Anviking a=Anviking # Issue Number ADP-1025 # Background #2763 made the wallet demand NodeToClientV_9. With cardano-node 1.28.0, V_9 is only enabled if `TestEnableDevelopmentNetworkProtocols` is set in the node config. # Overview - [x] Make the wallet support both `V_8` and `V_9`. - [x] Ensure integration tests on Mary are run without `TestEnableDevelopmentNetworkProtocols: True` # Comments <!-- 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) ✓ Jira will detect and link to this PR once created, but you can also link this PR in the description of the corresponding ticket ✓ Acknowledge any changes required to the Wiki ✓ Finally, in the PR description delete any empty sections and all text commented in <!--, so that this text does not appear in merge commit messages. --> 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-1025
Overview
[ ]Test that addresses don't affect the min ada valueExisting integration tests should be enoughDoes it still work on testnet? -> The ledger function in question cannot know the current network, so would be highly surprising if it didn't work on testnet[ ] Using cardano-node 1.28.0 tag-> stack.yaml: Bump cardano-node to 1.28.0 tag #2778[x] Cabal build also works-> Fix cabal build #2776Comments
step=cabalwhen creating a new build)