Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add collateralization & examples #1
base: main
Are you sure you want to change the base?
Add collateralization & examples #1
Changes from 10 commits
b89c748
b5aac2d
b5326a9
85a84f6
ac78864
b5808e6
e01b029
f58cf61
f621d5d
5994d51
2b14690
05995d6
fa4249f
82effe0
b96f3fc
452979c
c708eeb
033ab68
204fa2a
1bb91cf
3828ed9
7dd428a
95b30ea
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
nit/suggestion: Event names could be a bit more verbose, e.g. DepositCreated, DepositLocked, etc.
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.
I'm thinking this way to create IDs is unpredictable, since the resulting ID can be larger than expected if someone creates a deposit in the same block. So it might be a good idea to instead use a per-sender predictable ID e.g.
keccak256(msg.sender, nonces[msg.sender])
so that a caller can batch a deposit + lock or getDeposit or some other action knowing what the ID will be.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.
And a
depositFrom
function could be useful for users to create deposits on behalf of other depositors (as long as they have a token approval)Edit: actually, I think the opposite may be more useful: a
depositFor
ordepositTo
where a third party starts a deposit on behalf of someone else, providing the tokens. We have astakeTo
in the Staking contract that serves this purpose. This could be useful, among other things, if a service provider has tokens on several accounts but uses a single address to provide services.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.
Echoing Pablo's comments on IDs, I would also add
block.chainId
to the id generation/derivation.I can't think of any potential problems now, but since we will be building on top of this I think it's better to add it and rule out any silly cross-chain replay attacks.
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.
aaah good point, making deposit IDs unique cross-chain could prove useful in the future
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.
Can you elaborate what this is/does and why it's
onlyOwner
?