-
Notifications
You must be signed in to change notification settings - Fork 53
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
Create domain models #110
Create domain models #110
Conversation
function getMetadata () { | ||
return { | ||
public_key: config.getIn(['keys', 'ed25519', 'public']), | ||
urls: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "urls" object should probably be added by the controller since it's an HTTP thing. And for clarity this method could be renamed to something like "getDomainMetadata", which then gets merged with HTTP metadata.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this PR, I only moved as much of the existing code as I could without doing any refactoring. I plan to do anything else in a followup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some places where it's more difficult to separate the HTTP concerns from the domain layer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well this whole pull request is refactoring. It's a little strange to move something if you intend to move it right back.
LGTM after rename of |
Some of the payment stuff is getting removed by #109 anyway (they're going to conflict). |
@sentientwaffle I'll wait until that is merged and resolve the conficts |
@LumberJ thanks! |
First step for creating a layered architecture for connector interledger-deprecated/five-bells-ledger#112
7fbd757
to
9e65f7a
Compare
@clark800 updated |
First step for creating a layered architecture for connector
interledger-deprecated/five-bells-ledger#112