Skip to content

Conversation

@WhoSoup
Copy link
Member

@WhoSoup WhoSoup commented May 6, 2020

Tandem PR with FactomProject/factomd#996

This adds functions to the API to retrieve the various blocks without the raw component, making use of the above PR. Ideally, both are merged but this PR will work on its own, it just won't be any more efficient.

Each GetXBlock and GetXBlockByHeight function has a GetSimpleXBlock and GetSimpleXBlockByHeight counterpart with a different return signature. There are unit tests for all functions that check the return value is identical.

carryforward and others added 2 commits December 11, 2019 09:56
Release the V1 rollup to master.  This is used by factom-walletd version 2.2.16 and some pegnet software
@WhoSoup WhoSoup mentioned this pull request Aug 13, 2020
dblock.go Outdated
func getDBlock(keymr string, noraw bool) (dblock *DBlock, raw []byte, err error) {
params := keyMRRequest{KeyMR: keymr}
req := NewJSON2Request("directory-block", APICounter(), params)
req := NewJSON2Request("directory-block", APICounter(), params) // doesn't have a "no raw" request
Copy link
Contributor

Choose a reason for hiding this comment

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

why not? Isn't it up to use to introduce it? that's what we will be doing for other APIs no?

Copy link
Member Author

Choose a reason for hiding this comment

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

If someone specifically wants the DBlock (or any other block) raw data, they can get them via the "raw-data" method. The ones addressed in this PR are the ones where it sends a bunch of extra (and possibly useless) information with no way of turning it off. Ideally the way to unify them would be to remove the "raw" response from the ones in this PR but that might have too many adverse effects on people using the lib. This was a compromise that had the least incompatibility.

If we're doing breaking changes, I would go with removing the raw response entirely and relying on "raw-data" for those.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it. Then I think I'd be ok with making the big move of removing the raw data altogether from those APIs. I see that 0.4 as a way to get us back up and running and I am not against some bold moves (as long as they are documented). But maybe we can also see it as a preliminary release and make those big moves in the next iteration if you think we need more time to consolidate multiple important breaking changes. Any preference?

Copy link
Member Author

Choose a reason for hiding this comment

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

The factom lib isn't really stuck the way wax is, just hasn't had a release in a while. My preference would be to get these breaking changes into 0.4.

Copy link
Contributor

Choose a reason for hiding this comment

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

Great, let's do it!

@WhoSoup WhoSoup changed the base branch from master to develop August 18, 2020 10:29
@WhoSoup
Copy link
Member Author

WhoSoup commented Aug 18, 2020

Reworked to remove raw requests and changed base to develop.

@PaulBernier PaulBernier merged commit 70afea5 into FactomProject:develop Aug 19, 2020
PaulBernier pushed a commit that referenced this pull request Dec 19, 2020
* add noraw option

* use noraw param

* add noraw option

* use noraw param

* remove raw data request

* remove unnecessary comment

Co-authored-by: Brian Deery <carryforward@users.noreply.github.com>
PaulBernier pushed a commit that referenced this pull request Dec 19, 2020
* add noraw option

* use noraw param

* add noraw option

* use noraw param

* remove raw data request

* remove unnecessary comment

Co-authored-by: Brian Deery <carryforward@users.noreply.github.com>
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.

3 participants