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

feat(cactus-core-api): added new htlc aspect #587

Closed
wants to merge 1 commit into from

Conversation

jordigiam
Copy link

Resolve #586

Signed-off-by: jordigiam <jordi.giron.amezcua@accenture.com>
@jordigiam jordigiam changed the title feat(cactus-core-api): add new htlc aspect feat(cactus-core-api): added new htlc aspect Feb 19, 2021
@kikoncuo kikoncuo marked this pull request as ready for review February 19, 2021 08:37
@takeutak
Copy link
Member

takeutak commented Feb 22, 2021

@jordigiam Thank you for your proposal. I think the idea of HTLC itself is interesting, and when I see the issues, I think it is well planned.
However, regarding this plan, the HTLC application layer issue and the positioning of HTLC as a function of the Cactus platform are important issues, and I would like to discuss them in the next contributors' meeting.
I would like to review this PR after the contributors' opinion is reached. So, would it be possible for you to wait until next week for approval?

@jordigiam
Copy link
Author

@takeutak Sure! No problem!

@petermetz petermetz enabled auto-merge (rebase) February 24, 2021 15:01
@petermetz
Copy link
Member

Related art:
#580
#582

@petermetz petermetz disabled auto-merge March 11, 2021 17:52
@petermetz
Copy link
Member

@sfuji822 @takeutak Been on a while on this one. Could you please put it down here how would you change this? (That's the last thing I remember us talking about on one of the maintainer meetings where the conversation was cut short because we ran over time)

@takeutak
Copy link
Member

takeutak commented Mar 30, 2021

@petermetz We discussed on this topic on the previous meeting, but I'm sorry to be late to post the contents on this PR comment.
As we discuss about this PR on maintainer call 2021-03-16, we decide to move HTLC feature on core-api to extension ledger-specific feature on smart contract on connected ledgers.
Core-api is managed to deal with abstract operation independent on each ledger, and each ledger deal with ledger-specific operation as much as possible.

@petermetz
Copy link
Member

@takeutak I would say that the mentioned move has been performed in the PRs that hold the HTLC implementation and this one only contains what is considered abstract operation because the only change in here is the addition of a new item to an enum type which is meant to enable the implementations that reside in the other packages. Without this enum change the implementation packages won't compile because the PluginAspect enum is needed for it. Any chance that I managed to convince you? :-)

@petermetz
Copy link
Member

petermetz commented Apr 7, 2021

@sfuji822 @takeutak Kindly requesting that you review or follow-up on the review discussion within this pull request (whichever is applicable). Thank you very much in advance.

@takeutak
Copy link
Member

takeutak commented Apr 17, 2021

@petermetz Sorry for late replying. I was told by Peter's agenda that we would discuss this at the previous meeting, but in the end, the discussion turned to sprint development in the middle of the meeting, and I didn't have time to discuss it. Would it be possible for you to explain about this PR at the next meeting? If you do so, we will reach an agreement.

@petermetz
Copy link
Member

@takeutak

Sorry for late replying.

No problem, thank you for replying.

I was told by Peter's agenda that we would discuss this at the previous meeting, but in the end, the discussion turned to sprint development in the middle of the meeting, and I didn't have time to discuss it.

The previous meeting's agenda said that it was about planning specifically:

If we are left with time after the planning then continue with the topics that we didn't have enough time to finish in the last meeting

https://wiki.hyperledger.org/display/cactus/2021-04-13+Cactus+Maintainers+Agenda+-+Planning+Session

Would it be possible for you to explain about this PR at the next meeting?

The agenda for the next meeting is quite full already so we might not get to this one.
I can explain it here right now though (see below paragraph) and that way we can make progress on the discussion asynchronously without having to be blocked by the limited meeting time.
It's also important to make progress on the discussion asynchronously/as soon as possible because this pull request has been open for 2 months now.
Another reason why I think it would be great if we could have the discussion here is because it lowers the probability of misunderstandings.

PR explanation:

It's a one liner change adding a plugin aspect enum entry with the HTLC value so that plugins can be filtered/differentiated based on what is the aspect that they are solving.

If you feel that instead of HTLC it should be called something more generic like ATOMIC_SWAP or even more generic SMART_CONTRACT then I can't say I disagree, I also considered those values myself, naming things is hard (for me at least).


Did this explanation help at all? If not, would you mind letting me know specifically which part didn't make sense and then I'll try to expand further based on that.

@takeutak
Copy link
Member

@petermetz Thank you for your comment.
I reviewed it again including the latest HTLC PR (PR 873), but it seems like I am just throwing away the HTLC smart contract as I deploy it, and it is hard to understand the positioning of your HTLC, which is frankly what I am talking about.
I am sorry but I will have a vacation until next week, but I want to prepare the alternative plan as soon as possible. I am sorry to have kept you waiting so long.

@petermetz
Copy link
Member

@takeutak Thank you too!

but it seems like I am just throwing away the HTLC smart contract as I deploy it

Throwing it away as in deleting it? From the ledger?

@petermetz
Copy link
Member

@jordigiam FYI #885

@petermetz
Copy link
Member

@takeutak Never mind.

@petermetz petermetz closed this May 5, 2021
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.

feat(cactus-core-api): add new htlc aspect
3 participants