-
Notifications
You must be signed in to change notification settings - Fork 441
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
Update ERC-6123: EIP-6123, align EIP with local development #679
base: master
Are you sure you want to change the base?
Conversation
…tTriggered event), added two missing events in ISDC.sol
I need to have a look, because I thought that we discussed about theses events and I think the outcome would be that it is handled by the settlement token. I will check now. |
I believe the change to ISDC is not needed / superfluous. Remarks on We started distiguishing events on the Trade and events on the Settlement. For an SDC that has multiple trades all trade may / should settle in a single settlement. So the name "TradeSettlementPhase" is wrong. It is a settlement phase. Hence the event should start with Settlement. The
marks the beginning of the settlement phase (from the perspective of the SDC - maybe the token has a finer granularity). So this should replace Remarks on There are already two events that mark the end of the settlement phase:
|
Hi @cfries, TradeSettled is now more granular, that looks good, I will remove TradeSettled |
You are right. But there is already an event issued from performSettlement. The documentation in the comment already states that is is emitted when the settlement phase begins:
The method
The name starts with "Settlement" due to the remark I made above. Settlement is indepent of Trade. I believe I made the renaming comments somewhere, but it was a time ago. Maybe in Teams? :-) |
…ttlementEvaluated
okay, SettlementEvaluated is also fine for me and has some more details. But I cant find any "PaymentTriggered" event currently and we will need it on one of the token interfaces cause it could/will be necessary on more than one possible token implemenation |
… missing init functions to the interface
… missing init functions to the interface
Hi. @Julius278 - Why do we need to specify a separate interface for the offchain variant. One could include the PaymentTriggered Event into the ERC20TokenSettlement. Not sure whether we need a separate function "initParty". @cfries - this might be a bit subtle but stumbling across the settlement event names: SettlementRequested, SettlementEvaluated, SettlementTransferred. Since we removed the "Valuation" out of the SettlementRequested, we could generalise the second event naming also to be even more generic: SettlementEvaluated --> SettlementDetermined. |
Hi @pekola, "initParty" is not directly required in case of onchain settlement but it can be used as an "who is this party / address" description. |
|
added missing PaymentTriggered event