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

Add new LinearCodeAddressGenerator contract #467

Merged
merged 15 commits into from
Mar 3, 2025

Conversation

turbolent
Copy link
Member

Port the linear code address generator from https://github.com/onflow/flow-go/blob/master/model/flow/address.go to Cadence.

We'll likely need this for the migration code of onflow/cadence#3584, which will be implemented as a Cadence contract.

Copy link
Contributor

@sisyphusSmiling sisyphusSmiling left a comment

Choose a reason for hiding this comment

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

Left a couple clarifying questions. My main question is whether index needs to be tracked in contract or some struct's state or if that's relegated to some lower-level state.

@sisyphusSmiling
Copy link
Contributor

Approved but CI failing. Looks like the go assets need to be updated as well.

@tarakby
Copy link
Contributor

tarakby commented Nov 28, 2024

I'm back from time off and looking at this today

Copy link
Contributor

@tarakby tarakby left a comment

Choose a reason for hiding this comment

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

Nice!

@turbolent turbolent requested a review from tarakby November 29, 2024 18:19
@bluesign
Copy link
Contributor

bluesign commented Nov 30, 2024

Nit: I think address(index: 0) should fail

@tarakby
Copy link
Contributor

tarakby commented Dec 2, 2024

Nit: I think address(index: 0) should fail

right thanks @bluesign.
Actually, address(index) should only work for index in the range [1 .. 2^45 - 1], and should fail otherwise.

@turbolent
Copy link
Member Author

@tarakby Added a check for the index in 7fb76ed

@turbolent turbolent requested a review from tarakby December 2, 2024 18:03
Copy link
Contributor

@tarakby tarakby left a comment

Choose a reason for hiding this comment

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

Sweet! Thank you @turbolent, feel free to merge

@joshuahannan
Copy link
Member

okay. Can you add a little blurb about it in the README with intended import addresses also like there is for other contracts? Once that is added, I'm okay with you merging it

@turbolent
Copy link
Member Author

turbolent commented Feb 3, 2025

@joshuahannan Added the contract to the README. I don't know where the code will be deployed, we can add a table once it is. I initially ported the code assuming it would be needed for some migration code in flow-go, but we don't need it anymore. Even though we don't need it for that purpose, I feel like the functionality is still useful. Maybe it can be deployed to the service account? WDYT?

@joshuahannan
Copy link
Member

yeah, I think the service account makes sense. I'm fine with merging this and deploying it on testnet as soon as you think it is ready.

@turbolent
Copy link
Member Author

@joshuahannan It's ready from my side. It would be nice to merge and close this PR, as it's been open for a while.

Copy link
Member

@joshuahannan joshuahannan left a comment

Choose a reason for hiding this comment

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

Where is this code used? Does it need to be deployed to flow-go bootstrapping? If so, we should create a getter for it in the lib/go/contracts package. I can do that if needed

@turbolent
Copy link
Member Author

Where is this code used? Does it need to be deployed to flow-go bootstrapping?

The code is not used anywhere. As I mentioned above, I had originally created it / ported it from the Go code for the migration of on-the-fly account storage format in flow-go, but we did not end up needing it after all.
It is not part of the bootstrapped contracts and does not need a getter, but that could be done if someone needs it.

I still think it is useful to have the logic available on-chain, as a system contract that can be trusted.

@joshuahannan
Copy link
Member

Sounds like it probably should be included then so we can deploy it as part of flow-go bootstrapping and probably deploy it to testnet and mainnet. I'll add that

@joshuahannan joshuahannan merged commit 1102446 into master Mar 3, 2025
2 checks passed
@joshuahannan joshuahannan deleted the bastian/linear-code-address-generator branch March 3, 2025 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants