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

fixes #727

Merged
merged 4 commits into from
Oct 20, 2021
Merged

fixes #727

merged 4 commits into from
Oct 20, 2021

Conversation

tolikzinovyev
Copy link
Contributor

No description provided.

@tolikzinovyev tolikzinovyev requested a review from winder October 8, 2021 22:40
@tolikzinovyev tolikzinovyev self-assigned this Oct 8, 2021
@tolikzinovyev tolikzinovyev force-pushed the tolik/fixes branch 2 times, most recently from 71e1183 to 1d6b9d7 Compare October 8, 2021 22:54
@codecov-commenter
Copy link

codecov-commenter commented Oct 8, 2021

Codecov Report

Merging #727 (c946a6b) into develop (4be382e) will decrease coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #727      +/-   ##
===========================================
- Coverage    54.00%   53.97%   -0.03%     
===========================================
  Files           23       23              
  Lines         3674     3672       -2     
===========================================
- Hits          1984     1982       -2     
  Misses        1429     1429              
  Partials       261      261              
Impacted Files Coverage Δ
idb/postgres/internal/writer/writer.go 83.56% <ø> (ø)
idb/postgres/postgres.go 46.31% <ø> (-0.08%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4be382e...c946a6b. Read the comment docs.

Copy link
Contributor

@brianolson brianolson left a comment

Choose a reason for hiding this comment

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

lots of good, one comment

@@ -50,7 +50,7 @@ CREATE TABLE IF NOT EXISTS account (
created_at bigint NOT NULL, -- round that the account is first used
closed_at bigint, -- round that the account was last closed
keytype varchar(8), -- sig, msig, lsig, or null if unknown
account_data jsonb NOT NULL -- trimmed AccountData that excludes the fields above and the four creatable maps
account_data jsonb NOT NULL -- trimmed AccountData that excludes the fields above and the four creatable maps; "null" iff account is deleted
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this needs a better statement about how it is both NOT NULL and "null"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you think there needs to be an explanation in all five places or just the first one?

Copy link
Contributor

Choose a reason for hiding this comment

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

The first time let's say SQL 'NOT NULL' is held though the javascript string will be 'null' iff asset is deleted
After that just js string "null" iff asset is deleted

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, done

"VALUES ($1, 0, 0, 0, false, 0)"
query := `INSERT INTO account
(addr, microalgos, rewardsbase, rewards_total, deleted, created_at, account_data)
VALUES ($1, 0, 0, 0, false, 0, 'null'::jsonb)`
Copy link
Contributor

Choose a reason for hiding this comment

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

Am I reading this right? 'null'::jsonb is a valid jsonb syntax alternative like {} but null instead of empty?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes!

@tolikzinovyev tolikzinovyev merged commit 4307826 into develop Oct 20, 2021
@tolikzinovyev tolikzinovyev deleted the tolik/fixes branch October 20, 2021 15:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants