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

transfer-contract: moves state to the transfer-data contract #1247

Closed
wants to merge 0 commits into from
Closed

transfer-contract: moves state to the transfer-data contract #1247

wants to merge 0 commits into from

Conversation

miloszm
Copy link
Contributor

@miloszm miloszm commented Jan 9, 2024

Makes transfer-contract a logic-only (stateless) contract and moves all state data to the transfer-data contract.
Implements #1235

@miloszm miloszm force-pushed the issue-1235-rebased branch 2 times, most recently from 04adb54 to 9f48dbe Compare January 10, 2024 09:15
@miloszm miloszm marked this pull request as ready for review January 10, 2024 09:39
@miloszm miloszm requested a review from herr-seppia January 10, 2024 09:58
herr-seppia
herr-seppia requested changes Jan 10, 2024
View reviewed changes
Copy link
Member

@herr-seppia herr-seppia left a comment

Choose a reason for hiding this comment

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

Please split the commits accordingly to their workspace member

herr-seppia
herr-seppia requested changes Jan 10, 2024
View reviewed changes
contracts/transfer-data/src/lib.rs Outdated Show resolved Hide resolved
@miloszm miloszm marked this pull request as draft January 10, 2024 12:14
@miloszm miloszm force-pushed the issue-1235-rebased branch from d8d957c to 99f0ff6 Compare January 10, 2024 17:55
@miloszm miloszm marked this pull request as ready for review January 10, 2024 17:55
@miloszm miloszm requested a review from herr-seppia January 10, 2024 18:36
herr-seppia
herr-seppia requested changes Jan 11, 2024
View reviewed changes
Copy link
Member

@herr-seppia herr-seppia left a comment

Choose a reason for hiding this comment

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

In addition to what commented about ICC security, please split your commit properly.

You shouldn't touch ~/Makefile and ~/contracts/transfer-data/Makefile in the same commit (and prefix the commit with transfer-data)
Exactly as you shouldn't touch ~/Cargo.toml, ~/rusk-abi/*, ~/rusk-recovery/* and ~/stake/* in the transfer-data: initial check-in commit

contracts/transfer-data/rustfmt.toml Outdated Show resolved Hide resolved
contracts/transfer-data/src/lib.rs Outdated Show resolved Hide resolved
@miloszm miloszm marked this pull request as draft January 11, 2024 09:45
@miloszm miloszm force-pushed the issue-1235-rebased branch 2 times, most recently from a6fa9dd to 193e5dc Compare January 18, 2024 15:06
@miloszm miloszm marked this pull request as ready for review January 18, 2024 15:07
@miloszm miloszm force-pushed the issue-1235-rebased branch 2 times, most recently from 3b91c04 to 550000f Compare January 18, 2024 17:26
@miloszm miloszm force-pushed the issue-1235-rebased branch 5 times, most recently from 73cdc25 to 2e66268 Compare January 19, 2024 11:23
@miloszm miloszm requested review from ZER0, fed-franz and moCello January 19, 2024 11:24
fed-franz
fed-franz requested changes Jan 22, 2024
View reviewed changes
Copy link
Contributor

@fed-franz fed-franz left a comment

Choose a reason for hiding this comment

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

I think the logic of who has access to mint should be clarified.
Few typos to fix

The rest LGTM

contracts/transfer/src/lib.rs Outdated Show resolved Hide resolved
contracts/transfer/src/state.rs Outdated Show resolved Hide resolved
contracts/transfer-data/src/state.rs Outdated Show resolved Hide resolved
contracts/transfer-data/src/state.rs Outdated Show resolved Hide resolved
ureeves
ureeves requested changes Jan 22, 2024
View reviewed changes
Copy link
Member

@ureeves ureeves left a comment

Choose a reason for hiding this comment

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

I have some comments, especially related to the assertions of who is the caller, but I also feel like we shouldn't merge this to master, and it should be kept in something like a proxy branch.

contracts/stake/src/lib.rs Outdated Show resolved Hide resolved
contracts/stake/src/lib.rs Outdated Show resolved Hide resolved
contracts/transfer-data/src/lib.rs Outdated Show resolved Hide resolved
contracts/transfer-data/src/tree.rs Outdated Show resolved Hide resolved
fed-franz
fed-franz requested changes Jan 23, 2024
View reviewed changes
Copy link
Contributor

@fed-franz fed-franz left a comment

Choose a reason for hiding this comment

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

I'm happy with the changes. Jut a panic message to update

contracts/transfer/src/state.rs Outdated Show resolved Hide resolved
@miloszm miloszm requested review from ureeves and fed-franz January 23, 2024 15:56
fed-franz
fed-franz previously approved these changes Jan 23, 2024
View reviewed changes
@herr-seppia
Copy link
Member

I have some comments, especially related to the assertions of who is the caller, but I also feel like we shouldn't merge this to master, and it should be kept in something like a proxy branch.

Agree

ureeves
ureeves requested changes Jan 24, 2024
View reviewed changes
Copy link
Member

@ureeves ureeves left a comment

Choose a reason for hiding this comment

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

I would like to change the naming of owner_raw or add new function in rusk-abi that do (de)serialization.

rusk-abi/src/abi.rs Outdated Show resolved Hide resolved
@miloszm miloszm force-pushed the issue-1235-rebased branch 3 times, most recently from a083974 to bbb1354 Compare January 24, 2024 16:16
@miloszm miloszm requested a review from ureeves January 24, 2024 16:34
ureeves
ureeves previously approved these changes Jan 24, 2024
View reviewed changes
Copy link
Member

@ureeves ureeves left a comment

Choose a reason for hiding this comment

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

LGTM

herr-seppia
herr-seppia requested changes Jan 25, 2024
View reviewed changes
Copy link
Member

@herr-seppia herr-seppia left a comment

Choose a reason for hiding this comment

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

I would like to have tests for all "protected" method that verify that transactions are failing (like we actually have for "mint")

@miloszm miloszm force-pushed the issue-1235-rebased branch from cac3baf to b9d96bb Compare February 1, 2024 09:47
@miloszm miloszm requested a review from herr-seppia February 1, 2024 09:51
herr-seppia
herr-seppia requested changes Feb 2, 2024
View reviewed changes
rusk/tests/services/protected.rs Outdated Show resolved Hide resolved
herr-seppia
herr-seppia requested changes Feb 2, 2024
View reviewed changes
rusk/tests/services/protected.rs Outdated Show resolved Hide resolved
@miloszm miloszm force-pushed the issue-1235-rebased branch 2 times, most recently from c7647a4 to 9fb8a77 Compare February 6, 2024 15:57
@miloszm miloszm requested a review from herr-seppia February 6, 2024 16:10
@miloszm miloszm force-pushed the issue-1235-rebased branch from 9fb8a77 to 0b67d73 Compare February 6, 2024 16:22
@ureeves ureeves mentioned this pull request Feb 10, 2024
Open
@miloszm miloszm closed this Feb 15, 2024
@miloszm miloszm mentioned this pull request Feb 15, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ureeves ureeves ureeves left review comments

@fed-franz fed-franz fed-franz left review comments

@ZER0 ZER0 Awaiting requested review from ZER0

@moCello moCello Awaiting requested review from moCello

@herr-seppia herr-seppia Awaiting requested review from herr-seppia

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

Successfully merging this pull request may close these issues.
4 participants
@miloszm @herr-seppia @fed-franz @ureeves