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

1527 conway offchain voting #1562

Merged
merged 16 commits into from
Dec 20, 2023
Merged

1527 conway offchain voting #1562

merged 16 commits into from
Dec 20, 2023

Conversation

Cmdv
Copy link
Contributor

@Cmdv Cmdv commented Nov 16, 2023

Description

Add OffChain Voting Anchor utilising existing offchain code for pools
this fixes #1527

Checklist

  • Commit sequence broadly makes sense
  • Commits have useful messages
  • New tests are added if needed and existing tests are updated
  • Any changes are noted in the changelog
  • Code is formatted with fourmolu on version 0.10.1.0 (which can be run with scripts/fourmolize.sh)
  • Self-reviewed the diff

Migrations

  • The pr causes a breaking change of type a,b or c
  • If there is a breaking change, the pr includes a database migration and/or a fix process for old values, so that upgrade is possible
  • Resyncing and running the migrations provided will result in the same database semantically

If there is a breaking change, especially a big one, please add a justification here. Please elaborate
more what the migration achieves, what it cannot achieve or why a migration is not possible.

@Cmdv Cmdv force-pushed the 1527-conway-offchain-voting branch 3 times, most recently from a28398e to b69e7b7 Compare December 5, 2023 10:58
@Cmdv Cmdv force-pushed the 1527-conway-offchain-voting branch 2 times, most recently from 65e32b7 to 96720a3 Compare December 8, 2023 13:15
@Cmdv Cmdv marked this pull request as ready for review December 8, 2023 18:57
@Cmdv Cmdv requested review from a team as code owners December 8, 2023 18:57
kderme
kderme requested changes Dec 11, 2023
View reviewed changes
cardano-db-sync/src/Cardano/DbSync/OffChain/Http.hs Outdated Show resolved Hide resolved
cardano-db-sync/src/Cardano/DbSync/Era/Shelley/Insert.hs Outdated Show resolved Hide resolved
cardano-db-sync/src/Cardano/DbSync/OffChain.hs Outdated Show resolved Hide resolved
@Cmdv Cmdv force-pushed the 1527-conway-offchain-voting branch from edf95e5 to faa7cdf Compare December 12, 2023 09:09
kderme
kderme reviewed Dec 12, 2023
View reviewed changes
cardano-db-sync/src/Cardano/DbSync/OffChain.hs Show resolved Hide resolved
cardano-db-sync/src/Cardano/DbSync/Era/Shelley/Insert.hs Outdated Show resolved Hide resolved
cardano-db-sync/src/Cardano/DbSync/OffChain.hs Show resolved Hide resolved
cardano-db/src/Cardano/Db/Insert.hs Outdated Show resolved Hide resolved
cardano-db-sync/src/Cardano/DbSync.hs Outdated Show resolved Hide resolved
cardano-db/src/Cardano/Db/Types.hs Outdated Show resolved Hide resolved
cardano-db-sync/src/Cardano/DbSync/OffChain.hs Outdated Show resolved Hide resolved
@Cmdv Cmdv force-pushed the 1527-conway-offchain-voting branch 3 times, most recently from 06c59af to e6d33c7 Compare December 14, 2023 15:42
@Cmdv Cmdv requested a review from kderme December 15, 2023 09:42
kderme
kderme requested changes Dec 15, 2023
View reviewed changes
Copy link
Contributor

@kderme kderme 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.

Some remaining issues. Also I tested this on sanchonet and even though there are entries in voting_anchor, there are no entries in off_chain_vote_data or off_chain_vote_fetch_error.

cardano-db-sync/src/Cardano/DbSync/OffChain.hs Outdated Show resolved Hide resolved
cardano-db-sync/src/Cardano/DbSync/OffChain/Query.hs Show resolved Hide resolved
@kderme
Copy link
Contributor

kderme commented Dec 15, 2023 •
edited
Loading

A separate issue on sanchonet when off_chain_vote_data was inserted

cardano-db-sync: DbInsertException "OffChainVoteData" (SqlError {sqlState = "22P02", sqlExecStatus = FatalError, 
sqlErrorMsg = "invalid input syntax for type json", sqlErrorDetail = "The input string ended unexpectedly.", sqlErrorHint = ""})

Possibly the json sql type can't be empty

@Cmdv Cmdv force-pushed the 1527-conway-offchain-voting branch from ae2a9c4 to 90176da Compare December 15, 2023 14:25
@Cmdv Cmdv force-pushed the 1527-conway-offchain-voting branch from 90176da to 7ae6f39 Compare December 15, 2023 16:49
kderme
kderme requested changes Dec 17, 2023
View reviewed changes
cardano-db-sync/src/Cardano/DbSync/OffChain.hs Outdated Show resolved Hide resolved
cardano-db-sync/src/Cardano/DbSync/OffChain.hs Outdated Show resolved Hide resolved
cardano-db-sync/src/Cardano/DbSync/Era/Shelley/Insert.hs Outdated Show resolved Hide resolved
@kderme
Copy link
Contributor

kderme commented Dec 17, 2023

Could you add a new Changelog entry under sancho-next?

sgillespie
sgillespie reviewed Dec 18, 2023
View reviewed changes
cardano-db-sync/src/Cardano/DbSync/OffChain.hs
{-# LANGUAGE OverloadedStrings #-}
{-# LANGUAGE Rank2Types #-}
{-# LANGUAGE ScopedTypeVariables #-}
{-# LANGUAGE NoImplicitPrelude #-}
Copy link
Contributor

Choose a reason for hiding this comment

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

I notice we have a lot of modules using both Prelude and Cardano.Prelude. Should we use this pattern everywhere (NoImplicitPrelude)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

could set it globally in the cabal file? unless we need Prelude for some reason

kderme
kderme previously approved these changes Dec 20, 2023
View reviewed changes
@Cmdv Cmdv force-pushed the 1527-conway-offchain-voting branch from fe75715 to 489ee3d Compare December 20, 2023 11:58
kderme
kderme requested changes Dec 20, 2023
View reviewed changes
schema/migration-2-0034-20231220.sql Outdated Show resolved Hide resolved
cardano-db/src/Cardano/Db/Schema.hs

OffChainAnchorFetchError
Copy link
Contributor

Choose a reason for hiding this comment

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

These 2 tables that are deleted, should be deleted also from the sql files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

having a bit of trouble with the rebasing!! just reran a migration but it's doing weird things it seems

@Cmdv Cmdv force-pushed the 1527-conway-offchain-voting branch from 80e6a93 to 1a4a17d Compare December 20, 2023 12:55
kderme
kderme approved these changes Dec 20, 2023
View reviewed changes
Copy link
Contributor

@kderme kderme left a comment

Choose a reason for hiding this comment

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

LGTM!

@kderme kderme merged commit 3d3b5e4 into master Dec 20, 2023
16 of 22 checks passed
@Cmdv Cmdv mentioned this pull request Feb 5, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sgillespie sgillespie sgillespie left review comments

@kderme kderme kderme 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.

Get offchain Voting Anchor
3 participants
@Cmdv @kderme @sgillespie