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

Finalize unification or normalization of RPC calls #118

Merged
merged 2 commits into from
Jul 7, 2015
Merged

Finalize unification or normalization of RPC calls #118

merged 2 commits into from
Jul 7, 2015

Conversation

dexX7
Copy link
Member

@dexX7 dexX7 commented Jul 6, 2015

This pull request resolves #14.

The descriptions are based on actual output, which I tested.

Please let me know, if there is something to change or refine.

@dexX7 dexX7 force-pushed the oc-0.10-rpc-normalize-descriptions branch from f1ee0a5 to 54ea2f9 Compare July 6, 2015 23:24
@dexX7 dexX7 changed the title Continue documentation and normalization of RPC calls Finalize unification or normalization of RPC calls Jul 6, 2015
@dexX7
Copy link
Member Author

dexX7 commented Jul 6, 2015

The complete output of all help descriptions:

zathras-crypto
zathras-crypto reviewed Jul 7, 2015
View reviewed changes
src/omnicore/rpc.cpp
@@ -78,7 +78,7 @@ void MetaDexObjectToJSON(const CMPMetaDEx& obj, Object& metadex_obj)
// add data to JSON object
metadex_obj.push_back(Pair("address", obj.getAddr()));
metadex_obj.push_back(Pair("txid", obj.getHash().GetHex()));
if (obj.getAction() == 4) metadex_obj.push_back(Pair("ecosystem", isTestEcosystemProperty(obj.getProperty()) ? "Test" : "Main"));
metadex_obj.push_back(Pair("ecosystem", isTestEcosystemProperty(obj.getProperty()) ? "test" : "main"));
metadex_obj.push_back(Pair("propertyidforsale", (uint64_t) obj.getProperty()));
Copy link

Choose a reason for hiding this comment

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

Note, we don't provide an 'ecosystem' attribute in RPC output for any other call - the only reason it was included here was for cancel-by-ecosystem transactions to show what ecosystem the transaction applied to.

I don't really see the need to supply the ecosystem attribute except for that explicit case as it's an API and the ecosystem is easily devolved from already provided property ID data. But mostly I don't like doing it just for one type of transaction only (MetaDEx trades) - we should either supply property ID only (preferred) or both property ID and ecosystem for all transactions.

@zathras-crypto
Copy link

Couple of comments, other than that looks good to me - do you want to merge and then I'll redo my RPC PR to resolve conflicts?

@dexX7
Copy link
Member Author

dexX7 commented Jul 7, 2015

Yup, sounds good. One moment to address the comments.

@dexX7 dexX7 force-pushed the oc-0.10-rpc-normalize-descriptions branch from 54ea2f9 to 5ffd0c5 Compare July 7, 2015 01:43
@dexX7 dexX7 merged commit 5ffd0c5 into OmniLayer:omnicore-0.0.10 Jul 7, 2015
dexX7 added a commit that referenced this pull request Jul 7, 2015
@dexX7
Merge pull request #118
97f00c3 
5ffd0c5 Finalize normalization of RPC calls (dexX7)
cd7803e Leftovers of the rebranding to Omni Core (dexX7)
@zathras-crypto
Copy link

Thanks dude, I've resolved the conflicts but I'm getting a build failure here:

  CXX      omnicore/libbitcoin_server_a-notifications.o
In file included from omnicore/notifications.cpp:5:0:
./omnicore/version.h:44:37: error: ‘OMNICORE_VERSION_MILESTONE’ was not declared in this scope
                      +  100000000 * OMNICORE_VERSION_MILESTONE
                                     ^
./omnicore/version.h:45:37: error: ‘OMNICORE_VERSION_MAJOR’ was not declared in this scope
                      +     100000 * OMNICORE_VERSION_MAJOR
                                     ^
./omnicore/version.h:46:37: error: ‘OMNICORE_VERSION_MINOR’ was not declared in this scope
                      +        100 * OMNICORE_VERSION_MINOR
                                     ^
./omnicore/version.h:47:37: error: ‘OMNICORE_VERSION_PATCH’ was not declared in this scope
                      +          1 * OMNICORE_VERSION_PATCH;
                                     ^
Makefile:4338: recipe for target 'omnicore/libbitcoin_server_a-notifications.o' failed

@dexX7
Copy link
Member Author

dexX7 commented Jul 7, 2015

That's really strange. Did you change anything of your setup?

These OMNICORE_VERSION_X are defined just a few lines above:

... or, if exists, via src/config/bitcoin-config.h.

Can you try to run ./configure before building? I assume the config file exists, but is an outdated one from another branch.

@zathras-crypto
Copy link

Can you try to run ./configure before building? I assume the config file exists, but is an outdated one from another branch.

Yeah I'd done that already mate - let me poke around a bit more

@zathras-crypto
Copy link

So inside version.h we have:

#if defined(HAVE_CONFIG_H)
#include "config/bitcoin-config.h"
#else
......define omni core version stuff...
#endif

which means the omni core version stuff is only defined in version.h if we don't have a bitcoin-config.h file. I do have such a file, but the file does not contain any omni core version info.

I see what you're saying about it's perhaps an older version from another branch or something, but I've done a ./configure and make clean already...

Hmm...

@zathras-crypto
Copy link

I'll try deleting the file manually, then ./configure again. FYI my configure line has nothing special in it (just ./configure CPPFLAGS="-I${BDB_PREFIX}/include/ -O2" LDFLAGS="-L${BDB_PREFIX}/lib/" --with-gui

@zathras-crypto
Copy link

No joy, deleted the file & re=ran ./configure the file is recreated but it doesn't have any of the Omni Core version stuff (though it does have Omni Core package info). The file that is generated is here pastie.org/pastes/10276781/text?key=mqbr95yzjuw1bsjnczgig

@zathras-crypto
Copy link

Think I've got it sorted.

Needed to re-run ./autogen.sh before ./configure - the bitcoin-config.h file that was created after doing that does contain the Omni Core version defines.

Building now :)

@dexX7 dexX7 modified the milestone: 0.0.10.0 Dec 30, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
status: ready
Projects
None yet
Milestone
0.0.10.0
Development

Successfully merging this pull request may close these issues.
2 participants
@dexX7 @zathras-crypto