Skip to content

(VDB-1207) Remove non-essential smart comments#131

Merged
elizabethengelman merged 6 commits intostagingfrom
vdb-1207-smart-tag-based-documentation
Feb 24, 2020
Merged

(VDB-1207) Remove non-essential smart comments#131
elizabethengelman merged 6 commits intostagingfrom
vdb-1207-smart-tag-based-documentation

Conversation

@rmulhol
Copy link
Copy Markdown
Contributor

@rmulhol rmulhol commented Feb 20, 2020

This PR has morphed into removing smart tags for anything that's not a mutation and adding smart tags to omit mutations where we weren't doing so already.

Work on using tags for documentation/omitting specific read operations (e.g. attributes) has moved to https://github.com/makerdao/vdb-postgraphile/pull/4 where we'll have the ability to make continuous changes without modifying the database.


COMMENT ON TABLE maker.ilks
IS E'@name raw_ilks';
IS E'@name raw_ilks\nCDP type';
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should this say "Collateral type"? Although idk maybe "CDP type" will make more sense to people using the API.

);

COMMENT ON TABLE maker.vat_file_ilk
IS E'Note event emitted when file(bytes32,bytes32,uint256) is called on Vat contract';
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍

-- +goose StatementEnd

COMMENT ON FUNCTION api.all_urn_states(ilk_identifier TEXT, urn_identifier TEXT, block_height BIGINT, max_results INTEGER, result_offset INTEGER)
IS E'Get all historical states for an Urn prior to a given block. Ilk identifier (e.g. "ETH-A") and Urn identifier (e.g."") are required. Block height, limit, and offset are optional. Block height defaults to most recent block. Limit defaults to null (no limit). Offset defaults to 0.';
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Empty example Urn identifier ;)

@rmulhol rmulhol force-pushed the vdb-1207-smart-tag-based-documentation branch 3 times, most recently from 167e189 to 1d0492e Compare February 20, 2020 22:00
@rmulhol rmulhol changed the title [WIP] Add smart tags to migrations for documentation (VDB-1207) Add smart tags to migrations for documentation Feb 21, 2020
);

COMMENT ON TYPE api.flip_bid_snapshot
IS E'Historical snapshots of the state of auctions on the Flip contracts, with data about associated Urn.';
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I found it pretty difficult to distinguish between maker.flip and api.flip_bid_snaphot in the docs, and there are similar ambiguities (for me) on the other auctions contracts as well as urns. Wondering whether part of it might be that we were originally only going to expose the api schema and are now planning to expose all. If so, maybe it's worthwhile to consolidate? Alternatively, if anyone with a better understanding can chime in I'd be happy to update the docs.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah you're exactly right; the maker.flip/flap/flop tables were meant to be sort of intermediary caches, not exposed to the client. The client-facing functions like api.get_flip, api.all_flips etc. (which return snapshots) make use of those tables, and also do a couple more things at request-time, like determine whether the bid is dealt, and (in flip's case) figure out which ilk and urn are associated with the bid. Also, since only the api.-bid_snapshot types were meant to be client-facing, they have "computed columns", while the ones in the maker schema don't.

I agree that it would probably make sense to do some consolidation. To that end, it seems feasible to remove all the request-time logic that get_flip/flap/flop are doing, and instead add the bids' dealt, ilk, and urn data to our trigger tables. Then we could probably trim the API down to just using a single type for each kind of bid.

@rmulhol
Copy link
Copy Markdown
Contributor Author

rmulhol commented Feb 21, 2020

Currently working on removing all smart tags except for @omit tags on mutations (which I think we should never expose).

Instead I'm planning to move all smart tags over to the vdb-postgraphile repo so that we can modify them without touching migrations: https://github.com/makerdao/vdb-postgraphile/pull/4

@rmulhol rmulhol force-pushed the vdb-1207-smart-tag-based-documentation branch from 585b62a to 16f0f17 Compare February 21, 2020 22:08
Rob Mulholand added 5 commits February 21, 2020 16:14
- Currently exposed tables and queries in all schemas
- Except cases where they omit a mutation
- Moving them over to postgraphile.tags.json5 for easier updating
@rmulhol rmulhol force-pushed the vdb-1207-smart-tag-based-documentation branch from 16f0f17 to 2733d3c Compare February 21, 2020 22:14
@rmulhol rmulhol changed the title (VDB-1207) Add smart tags to migrations for documentation (VDB-1207) Remove non-essential smart comments Feb 21, 2020
Copy link
Copy Markdown
Contributor

@elizabethengelman elizabethengelman left a comment

Choose a reason for hiding this comment

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

LGTM!


COMMENT ON TABLE maker.vat_debt
IS E'@omit';
IS E'';
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

i think we can probably remove this line altogether now, right?

@elizabethengelman elizabethengelman merged commit 1943cbb into staging Feb 24, 2020
@elizabethengelman elizabethengelman deleted the vdb-1207-smart-tag-based-documentation branch February 24, 2020 21:25
elizabethengelman pushed a commit that referenced this pull request Feb 28, 2020
* Bump vDB version to 0.0.12-rc.1

* Fix alias for historical flop states

* VDB-1189: Add addresses to storage initializers (#124)

* Update vdb version to include Address in storage initializers

* Add contract addresses to storage initializers

* Remove HashedAddress in favor of Address in storage initializers

* (VDB-1164) Copy over migrations from vDB

* 1189 update transformer imports (#126)

* Update vdb version to include Address in storage initializers

* Update storage transformer imports

* Update transformerExporter.go to reflect new location of storage transformer interface

* Update event transformer imports

* Update transformerExporter.go to reflect new location of event transformer interface

* Update vdb version to include new location for transformer interfaces

* (VDB-1199) Add extractDiffs to docker-compose.yml

- Also remove unnecessary env data from other images

* Vdb 1200 get storage value dockerfile (#127)

* Add Dockerfile for getStorageValue cmd

* Move execute dockerfiles to their own directory

* TO-879: dockerhub via travis (#120)

* dockerhub via travis

* Update travis/deploy.sh to account for new path to execute dockerfile

* Build and push get-storage-value docker image in travis/deploy.sh

* (VDB-1207) Remove non-essential smart comments (#131)

* (VDB-1207) Add smart tags to document DB

- Currently exposed tables and queries in all schemas

* Remove unused code

* Omit queries that perform mutations

* Expose and document storage tables

* Remove smart tags from migrations

- Except cases where they omit a mutation
- Moving them over to postgraphile.tags.json5 for easier updating

* Remove an unused smart comment

Co-authored-by: Elizabeth <emengelman@gmail.com>

* Update postgres version in travis build (#132)

* Update postgres version in travis build to 11.6

this is the most recent posgres version available in RDS at the moment

* updates travis secrets (#133)

* Update vdb to v0.0.13-rc.1 (#134)

* Makes updates to dockerfile/README.md

* Copy header migration change from vdb (#136)

* (VDB-1202) OSM change event transformer

* Update vdb version to v0.0.13-rc.2 (#138)

Co-authored-by: Rob Mulholand <rmulholand@8thlight.com>
Co-authored-by: Gabe Laughlin <gabe@bistrotech.net>
Co-authored-by: daimesava <56128157+daimesava@users.noreply.github.com>
Co-authored-by: Andrew J Yao <1547809+yaoandrew@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants