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.

Sign up for GitHub

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

Jump to bottom

refactor: clean up NST exits related code #206

Merged
merged 2 commits into from
Jun 19, 2019
Merged

refactor: clean up NST exits related code #206

merged 2 commits into from
Jun 19, 2019

Conversation

troggy
Copy link
Member

@troggy troggy commented Jun 17, 2019

  • drop ExitStartedV2 event in favour of ExitStarted
  • put token data into Exit struct

Resolves #197, #198

@troggy
refactor: clean up NST exits related code
4bc1880 
- drop ExitStartedV2 event in favour of ExitStarted
- put token data into Exit struct
johannbarbie
johannbarbie reviewed Jun 17, 2019
View reviewed changes
contracts/ExitHandler.sol
);

/**
- tokenData — (optional) NST data
*/
struct Exit {
uint256 amount;
uint16 color;
address owner;
bool finalized;
uint32 priorityTimestamp;
uint256 stake;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you make stake a uint32 (for example multiple of 0.1 ETH), then you save one storage slot.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

out of scope, but noted here: #207

@johannbarbie
Copy link
Member

also this comes to mind: #208

@troggy troggy marked this pull request as ready for review June 18, 2019 18:16
@troggy troggy requested a review from johannbarbie June 19, 2019 12:10
@johannbarbie
Copy link
Member

is there a pr in node that reflects the changes in ABI?

troggy added a commit to leapdao/leap-node that referenced this pull request Jun 19, 2019
@troggy
feat: update bridge and operator abis
5ca1e42 
to conform to leapdao/leap-contracts#131 and
leapdao/leap-contracts#206
@troggy troggy mentioned this pull request Jun 19, 2019
Merged
@troggy
Copy link
Member Author

troggy commented Jun 19, 2019 •
edited
Loading

@johannbarbie leapdao/leap-node#281

includes the changes for leapdao/leap-node#131 as well (drops NewHeight event) not anymore (see #209 (comment))

@troggy
Copy link
Member Author

troggy commented Jun 19, 2019

testing this right now as you suggested in leapdao/leap-node#131

@troggy
Copy link
Member Author

troggy commented Jun 19, 2019 •
edited
Loading

Submission event signature is different for updated contracts, so the updated node won't catch Submission event from old contracts (before upgrade). Will figure out consequences and how to solve

@troggy
Copy link
Member Author

troggy commented Jun 19, 2019 •
edited
Loading

NewHeight event is not used in leap-node, so not a problem

johannbarbie
johannbarbie approved these changes Jun 19, 2019
View reviewed changes
Copy link
Member

@johannbarbie johannbarbie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💚

@troggy troggy merged commit 0aa613d into master Jun 19, 2019
@troggy troggy deleted the ref/197-198-NST-exits branch June 19, 2019 17:59
@troggy troggy mentioned this pull request Jun 19, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@johannbarbie johannbarbie johannbarbie approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

include exitsTokenData in Exit struct
2 participants
@troggy @johannbarbie