-
Notifications
You must be signed in to change notification settings - Fork 214
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
Update to cardano-node 1.35-rc3 #3340
Conversation
e759697
to
c434449
Compare
468807c
to
f9e8b1f
Compare
- Bump cardano-wallet to 1.35.0-rc3
f9e8b1f
to
b5fe3e1
Compare
- cardano-base - cardano-ledger - ouroboros-network
- 'evaluateTransactionExecutionUnits' has a new error type, update ErrAssignRedeemers accordingly. - 'SimpleScriptWitness' now takes a 'SimpleScriptOrReferenceInput' instead of a 'SimpleScript', update generators accordingly.
b5fe3e1
to
d0ef7df
Compare
-- We differentiate this from @TranslationError@ for partial API | ||
-- backwards compatibility. | ||
apiError err400 PastHorizon $ T.unwords | ||
[ "The transaction's validity interval is past the horizon" | ||
, "of safe slot-to-time conversions." | ||
, "of safe slot-to-time conversions: " <> t <> "." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooh, nice! We have gotten some details back again. I think the exact Text
comes from https://github.com/input-output-hk/cardano-wallet/blob/8e1b7239c090d3c110e8f24fb58396b5ec30327c/lib/shelley/src/Cardano/Wallet/Shelley/Transaction.hs#L1180 and is pretty long, so could we make this some "\n\n Here are the full details:" <> t
at the end?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. I was trying to write a test that would return this error (I was also worried about the readability), as chasing the definition of the error down proved more difficult than expected. But triggering the error also proved difficult. I will move it to the end 👍
-- Note that although this error is thrown from | ||
-- '_assignScriptRedeemers', it's more related to balanceTransaction | ||
-- in general than to assigning redeemers. Hence we don't mention | ||
-- redeemers in the message. | ||
apiError err400 UnresolvedInputs $ T.unwords | ||
[ "The transaction I was given contains inputs I don't know" | ||
, "about. Please ensure all foreign inputs are specified as " | ||
, "part of the API request. The unknown inputs are:\n\n" | ||
, pretty ins | ||
] | ||
ErrAssignRedeemersTranslationError TimeTranslationPastHorizon -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably TranslationLogicMissingInput
is the replacement for the removed UnknownTxIns
error, so:
Suggestion:
ErrAssignRedeemersTranslationError (TranslationLogicMissingInput inp) ->
-- Note that although this error is thrown from
-- '_assignScriptRedeemers', it's more related to balanceTransaction
-- in general than to assigning redeemers. Hence we don't mention
-- redeemers in the message.
apiError err400 UnresolvedInputs $ T.unwords
[ "The transaction I was given contains inputs I don't know"
, "about. Please ensure all foreign inputs are specified as "
, "part of the API request. The unknown input is:\n\n"
, T.pack $ show inp
]
then we should be able to keep the test (and this error code) unchanged. (Maybe it is in fact neater to more transparently only relying on TranslationError
, but in-doubt, minimising the API changes for now seems safer to me — let me know if you disagree)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some reason the test coverage seems very poor now. Not sure what's changed. I do get the new error, but just very rarely. I think it might be best to mark it pending / remove, and if anything have a simple unit test.
I pushed to a separate branch, feel free to cherry-pick if you see fit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, wanted to ask you about this 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As long as we preserve the UnresolvedInputs
API error for good measure and update the readme I think we should merge!
bors try |
tryBuild failed: |
- Move full details of "time translation past horizon" error to the end of the error message, in case the full details are long.
- Bump supported version of cardano-node in compatibility matrix to 1.35.0-rc3.
I've cherry picked your commits @Anviking so we can go ahead and merge. I had a better look into the coverage issue. I'm not sure why the coverage fell yet, but I can see a lot of coverage is lost to insufficient funds errors - might be worth re-jigging generators to be aware of the Wallet's UTxO to improve coverage. For now we should probably merge and I'll work on something like that - or a unit test. |
E.g., after some fixes to conflicting withdrawals, this is the sort of error coverage I get:
|
bors r+ |
3340: Update to cardano-node 1.35-rc3 r=Anviking a=sevanspowell I have: - Bumped the following dependencies to the `1.35.0-rc3` tag: - cardano-node - cardano-base - cardano-ledger - ouroboros-network - Updated cardano-wallet code to build with `1.35.0-rc3` tag: - `evaluateTransactionExecutionUnits` has a new error type, updated `ErrAssignRedeemers` accordingly. - `SimpleScriptWitness` now takes a `SimpleScriptOrReferenceInput` instead of a `SimpleScript`, updated generators accordingly. TODO: - [x] Consider whether the `prop_balanceTransactionUnresolvedInputs` test makes sense anymore. - [ ] Test the API output of `ErrAssignRedeemersTranslationError`. - [x] Update readme compatibility matrix ### Issue Number ADP-1907 Co-authored-by: Samuel Evans-Powell <[email protected]> Co-authored-by: Johannes Lund <[email protected]>
Build failed: |
bors r+ |
3340: Update to cardano-node 1.35-rc3 r=Anviking a=sevanspowell I have: - Bumped the following dependencies to the `1.35.0-rc3` tag: - cardano-node - cardano-base - cardano-ledger - ouroboros-network - Updated cardano-wallet code to build with `1.35.0-rc3` tag: - `evaluateTransactionExecutionUnits` has a new error type, updated `ErrAssignRedeemers` accordingly. - `SimpleScriptWitness` now takes a `SimpleScriptOrReferenceInput` instead of a `SimpleScript`, updated generators accordingly. TODO: - [x] Consider whether the `prop_balanceTransactionUnresolvedInputs` test makes sense anymore. - [ ] Test the API output of `ErrAssignRedeemersTranslationError`. - [x] Update readme compatibility matrix ### Issue Number ADP-1907 3341: Light mode: reduce concurrency when retrieving data. r=Unisay a=Unisay - [x] Replace concurrent traversals with sequential ones in order to minimize likelihood of hitting `Too Many Requests` error. - [x] Fix bug with fetching Tx hashes for the same block multiple times. ### Comments Unfortunately the "Too Many Requests" error shows up anyway: [wallet.log](https://github.com/input-output-hk/cardano-wallet/files/8918830/wallet.log) ### Issue Number ADP-1651 Co-authored-by: Samuel Evans-Powell <[email protected]> Co-authored-by: Johannes Lund <[email protected]> Co-authored-by: Yuriy Lazaryev <[email protected]> Co-authored-by: Yura Lazarev <[email protected]>
Build failed (retrying...): |
3340: Update to cardano-node 1.35-rc3 r=Anviking a=sevanspowell I have: - Bumped the following dependencies to the `1.35.0-rc3` tag: - cardano-node - cardano-base - cardano-ledger - ouroboros-network - Updated cardano-wallet code to build with `1.35.0-rc3` tag: - `evaluateTransactionExecutionUnits` has a new error type, updated `ErrAssignRedeemers` accordingly. - `SimpleScriptWitness` now takes a `SimpleScriptOrReferenceInput` instead of a `SimpleScript`, updated generators accordingly. TODO: - [x] Consider whether the `prop_balanceTransactionUnresolvedInputs` test makes sense anymore. - [ ] Test the API output of `ErrAssignRedeemersTranslationError`. - [x] Update readme compatibility matrix ### Issue Number ADP-1907 Co-authored-by: Samuel Evans-Powell <[email protected]> Co-authored-by: Johannes Lund <[email protected]>
Build failed: |
bors r+ |
3340: Update to cardano-node 1.35-rc3 r=Anviking a=sevanspowell I have: - Bumped the following dependencies to the `1.35.0-rc3` tag: - cardano-node - cardano-base - cardano-ledger - ouroboros-network - Updated cardano-wallet code to build with `1.35.0-rc3` tag: - `evaluateTransactionExecutionUnits` has a new error type, updated `ErrAssignRedeemers` accordingly. - `SimpleScriptWitness` now takes a `SimpleScriptOrReferenceInput` instead of a `SimpleScript`, updated generators accordingly. TODO: - [x] Consider whether the `prop_balanceTransactionUnresolvedInputs` test makes sense anymore. - [ ] Test the API output of `ErrAssignRedeemersTranslationError`. - [x] Update readme compatibility matrix ### Issue Number ADP-1907 Co-authored-by: Samuel Evans-Powell <[email protected]> Co-authored-by: Johannes Lund <[email protected]>
Build failed: packet-ipxe-3-ci3-1
and packet-ipxe-1-ci1-3
|
bors r+ |
3340: Update to cardano-node 1.35-rc3 r=Anviking a=sevanspowell I have: - Bumped the following dependencies to the `1.35.0-rc3` tag: - cardano-node - cardano-base - cardano-ledger - ouroboros-network - Updated cardano-wallet code to build with `1.35.0-rc3` tag: - `evaluateTransactionExecutionUnits` has a new error type, updated `ErrAssignRedeemers` accordingly. - `SimpleScriptWitness` now takes a `SimpleScriptOrReferenceInput` instead of a `SimpleScript`, updated generators accordingly. TODO: - [x] Consider whether the `prop_balanceTransactionUnresolvedInputs` test makes sense anymore. - [ ] Test the API output of `ErrAssignRedeemersTranslationError`. - [x] Update readme compatibility matrix ### Issue Number ADP-1907 Co-authored-by: Samuel Evans-Powell <[email protected]> Co-authored-by: Johannes Lund <[email protected]>
Build failed: |
bors r+ |
3340: Update to cardano-node 1.35-rc3 r=Anviking a=sevanspowell I have: - Bumped the following dependencies to the `1.35.0-rc3` tag: - cardano-node - cardano-base - cardano-ledger - ouroboros-network - Updated cardano-wallet code to build with `1.35.0-rc3` tag: - `evaluateTransactionExecutionUnits` has a new error type, updated `ErrAssignRedeemers` accordingly. - `SimpleScriptWitness` now takes a `SimpleScriptOrReferenceInput` instead of a `SimpleScript`, updated generators accordingly. TODO: - [x] Consider whether the `prop_balanceTransactionUnresolvedInputs` test makes sense anymore. - [ ] Test the API output of `ErrAssignRedeemersTranslationError`. - [x] Update readme compatibility matrix ### Issue Number ADP-1907 3341: Light mode: reduce concurrency when retrieving data. r=Unisay a=Unisay - [x] Replace concurrent traversals with sequential ones in order to minimize likelihood of hitting `Too Many Requests` error. - [x] Fix bug with fetching Tx hashes for the same block multiple times. ### Comments Unfortunately the "Too Many Requests" error shows up anyway: [wallet.log](https://github.com/input-output-hk/cardano-wallet/files/8918830/wallet.log) ### Issue Number ADP-1651 3344: Move `adjustNoSuchWallet` and `ErrNoSuchWallet` r=HeinrichApfelmus a=HeinrichApfelmus ### Issue number ADP-1043 ### Overview This pull request is a pure refactoring. We move `adjustNoSuchWallet` and `ErrNoSuchWallet` to the `Cardano.Wallet.DB.WalletState` module. Co-authored-by: Samuel Evans-Powell <[email protected]> Co-authored-by: Johannes Lund <[email protected]> Co-authored-by: Yuriy Lazaryev <[email protected]> Co-authored-by: Yura Lazarev <[email protected]> Co-authored-by: Heinrich Apfelmus <[email protected]>
Build failed (retrying...): |
3340: Update to cardano-node 1.35-rc3 r=Anviking a=sevanspowell I have: - Bumped the following dependencies to the `1.35.0-rc3` tag: - cardano-node - cardano-base - cardano-ledger - ouroboros-network - Updated cardano-wallet code to build with `1.35.0-rc3` tag: - `evaluateTransactionExecutionUnits` has a new error type, updated `ErrAssignRedeemers` accordingly. - `SimpleScriptWitness` now takes a `SimpleScriptOrReferenceInput` instead of a `SimpleScript`, updated generators accordingly. TODO: - [x] Consider whether the `prop_balanceTransactionUnresolvedInputs` test makes sense anymore. - [ ] Test the API output of `ErrAssignRedeemersTranslationError`. - [x] Update readme compatibility matrix ### Issue Number ADP-1907 Co-authored-by: Samuel Evans-Powell <[email protected]> Co-authored-by: Johannes Lund <[email protected]>
Build failed: |
bors r+ |
3340: Update to cardano-node 1.35-rc3 r=sevanspowell a=sevanspowell I have: - Bumped the following dependencies to the `1.35.0-rc3` tag: - cardano-node - cardano-base - cardano-ledger - ouroboros-network - Updated cardano-wallet code to build with `1.35.0-rc3` tag: - `evaluateTransactionExecutionUnits` has a new error type, updated `ErrAssignRedeemers` accordingly. - `SimpleScriptWitness` now takes a `SimpleScriptOrReferenceInput` instead of a `SimpleScript`, updated generators accordingly. TODO: - [x] Consider whether the `prop_balanceTransactionUnresolvedInputs` test makes sense anymore. - [ ] Test the API output of `ErrAssignRedeemersTranslationError`. - [x] Update readme compatibility matrix ### Issue Number ADP-1907 Co-authored-by: Samuel Evans-Powell <[email protected]> Co-authored-by: Johannes Lund <[email protected]>
Build failed: |
Unfortunately I can't reproduce the timeout bors is seeing locally, so think this is a |
bors r+ |
Build succeeded: |
I have:
1.35.0-rc3
tag:1.35.0-rc3
tag:evaluateTransactionExecutionUnits
has a new error type, updatedErrAssignRedeemers
accordingly.SimpleScriptWitness
now takes aSimpleScriptOrReferenceInput
instead of aSimpleScript
, updated generators accordingly.TODO:
prop_balanceTransactionUnresolvedInputs
test makes sense anymore.ErrAssignRedeemersTranslationError
.Issue Number
ADP-1907