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.

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

Small fixes and cleanup #133

Merged
merged 6 commits into from
Sep 26, 2022
Merged

Small fixes and cleanup #133

merged 6 commits into from
Sep 26, 2022

Conversation

guggero
Copy link
Member

@guggero guggero commented Sep 22, 2022

Fixes a few smaller things:

  • Bump sqlc version to v1.15.0 (can use new config format, but sqlite engine doesn't understand WITH clauses yet, so we can't use that).
  • Add missing tarocli addrs receives command to show inbound asset transfers.
  • Remove workaround that is no longer necessary in proof construction.
  • Fix the address to asset comparison that was patched incompletely.
  • Add the genesis bootstrap info to the assets displayed in the asset balance call responses.

A workaround was added during debugging to allow constructing a partial
proof when the block is not known yet.
This has been fixed in another way (creating a fake block on at the call
site) in the meantime, so this workaround is no longer needed here.
@guggero guggero marked this pull request as ready for review September 22, 2022 17:30
Copy link
Collaborator

@jharveyb jharveyb left a comment

Choose a reason for hiding this comment

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

Looks good!

queries: "tarodb/sqlite/queries"
version: "2"
sql:
- engine: "postgresql"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks equivalent, but why is the engine PostgreSQL here in general?

Copy link
Member

Choose a reason for hiding this comment

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

When we started the project, the sqlite engine wasn't ready. However turns out, that we can use the postgres engine as is, since we're not using any sqlite specific features yet. This also means we can ideally "easily" add another layer of scaffolding and then support postgres as well w/ minimal changes.


// If one of the family keys is not nil while the other one is, then we
// can already exit here as we know things won't match up further.
if !famKeyBothNil && !famKeyNoneNil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

IMO the other representation is easier to read b/c only one negation:

!(famKeyBothNil || famKeyNoneNil)

But not a blocker.


// TestAddrMatchesAsset tests that the AddrMatchesAsset function works
// correctly.
func TestAddrMatchesAsset(t *testing.T) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

💯

Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

LGTM 🧦

@Roasbeef Roasbeef merged commit 3206f82 into main Sep 26, 2022
@guggero guggero deleted the fixes-cleanup branch September 26, 2022 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants