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

Escrow contract: tweak the event TransactionCreated #1389

Closed
jaybuidl opened this issue Jan 3, 2024 · 4 comments · Fixed by #1396
Closed

Escrow contract: tweak the event TransactionCreated #1389

jaybuidl opened this issue Jan 3, 2024 · 4 comments · Fixed by #1396

Comments

@jaybuidl
Copy link
Member

jaybuidl commented Jan 3, 2024

Escrow.TransactionCreated event: add the parameters asset, _deadline and _transactionUri.

For example

event TransactionCreated(uint256 indexed _transactionID, 
  string _transactionUri, 
  address indexed _buyer, 
  address indexed _seller, 
  uint256 _amount, 
  string _asset, 
  uint256 _deadline
);

https://github.com/kleros/kleros-v2/blob/dev/contracts/src/arbitration/arbitrables/Escrow.sol#L90-L95

@kemuru
Copy link
Contributor

kemuru commented Jan 8, 2024

@jaybuidl with _asset you mean the token? but since this is the general escrow, it will always be main currency of the chain you're in right, so not necessary?

@kemuru kemuru linked a pull request Jan 8, 2024 that will close this issue
@jaybuidl
Copy link
Member Author

jaybuidl commented Jan 9, 2024

@jaybuidl with _asset you mean the token? but since this is the general escrow, it will always be main currency of the chain you're in right, so not necessary?

For the native currency, let have _asset = "native".

I want to avoid having a variable which looks the same as if it was uninitialized, such as address(0) or string(""), to make the value more explicit and intentional. Ofc in the case of a token, _asset is the token address.

@kemuru
Copy link
Contributor

kemuru commented Jan 9, 2024

@jaybuidl with _asset you mean the token? but since this is the general escrow, it will always be main currency of the chain you're in right, so not necessary?

For the native currency, let have _asset = "native".

I want to avoid having a variable which looks the same as if it was uninitialized, such as address(0) or string(""), to make the value more explicit and intentional. Ofc in the case of a token, _asset is the token address.

yeah makes sense but, wasn't the crypto swap going to be a different smart contract than this one? In my mind there was going to be 1 smart contract for General Escrow (with the only possibility of using native token) and a different smart contract for Crypto Swaps

@jaybuidl
Copy link
Member Author

jaybuidl commented Jan 9, 2024

yeah makes sense but, wasn't the crypto swap going to be a different smart contract than this one? In my mind there was going to be 1 smart contract for General Escrow (with the only possibility of using native token) and a different smart contract for Crypto Swaps

Yeah different contracts but same interface if possible

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants