Conversation
Change mostly facilitated by Claude.
|
All alerts resolved. Learn more about Socket for GitHub. This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. |
| # conflicts with other caching actions that might have cleaned some of cached contents. | ||
| path: svm-${{ inputs.type }}.tar | ||
| key: svm-${{ inputs.type }}-${{ runner.os }}-node-${{ steps.resolved-node.outputs.version }}-${{ hashFiles('Cargo.lock', 'programs/**/Cargo.toml', 'programs/**/Xargo.toml', 'programs/**/*.rs') }} | ||
| key: svm-${{ inputs.type }}-${{ runner.os }}-node-${{ steps.resolved-node.outputs.version }}-${{ hashFiles('yarn.lock', 'Cargo.lock', 'programs/**/Cargo.toml', 'programs/**/Xargo.toml', 'programs/**/*.rs') }} |
There was a problem hiding this comment.
yarn.lock added here to ensure that changes in codama versions will trigger a new cache key. Without this change there are type mismatches due to changes in kit between 2.3 and 3.0.
There was a problem hiding this comment.
I preferred to drop types unless they're required. Types are resolving OK dynamically on my end.
pxrl
changed the title
| commitment: Commitment = "confirmed" | ||
| ) => { | ||
| const signedTransaction = await signTransactionMessageWithSigners(transactionMessage); | ||
| assertIsTransactionWithinSizeLimit(signedTransaction); |
There was a problem hiding this comment.
before, the TX would fail. Now, we will panic here?
Also, I forget, do downstream repos rely on contracts repo to keep production-ready utils, or are these just for testing / prototyping?
Just for the future, it's the same thing that happened with hardhat / foundry.
Hardhat used to generate artifacts, and those became ingrained into all the downstream repos. Changing that became painful.
@Reinis-FRP @md0x do you guys know if there's any way to "export some minimal artifacts" from the contracts repo that e.g. the sdk will use to define some production utils? Then the contracts repo can worry only about the contracts, and the sdk can worry about maintaining the utils
There was a problem hiding this comment.
As far as I know sdk mainly depends on codama clients and anchor IDL/types that are generated and exported in contracts repo. Everything else is expected to be used for contracts testing and prototyping scripts here. I've seen some exceptions to this in sdk where we import some utils from contracts in sdk tests as well as some cctp decoding and some other conversion utils, but those should be agnostic to the solana/kit versioning.
There was a problem hiding this comment.
before, the TX would fail. Now, we will panic here?
We have a similar situation in the v3 -> v4 migration; I left some comments here: #1245 (comment)
There was a problem hiding this comment.
Yeah, I think what happened is that, in a few places (sdk and possibly others down the line) we ended up reusing functions from the contracts repo simply because it was the fastest way to ship instead of reimplementing them in the sdk.
I agree with the concern: ideally, any utils living in the contracts repo should be internal and for test only, not something downstream repos should use.
Related to that, one change that could make sense is moving all codama client generation logic into the sdk, we didn't intend to use them in the contracts. In the contracts repo those clients have mainly been useful in tests to validate functional equivalence between codama clients and anchor but they don’t necessarily need to live ther.
So I agree and align with your suggestions @pxrl @grasphoper : contracts should focus on contracts + correctness, and the sdk should have the utils and client for prod with a minimal interface (if any) from the contracts repo.
grasphoper
left a comment
grasphoper
left a comment
There was a problem hiding this comment.
LGTM, interested to discuss this #1243 (comment)
I.e., what repo should be maintaining the TS types and utils for contract interactions? Ideally we keep the contracts repo pure. But maybe that's impossible / impractical
Yeah, it currently lives here but I'm not sure if that's out of pure necessity, practicality, or just a byproduct of the way Solana support was initially added. We can take a discussion w/ @Reinis-FRP and @md0x to confirm whether we need this all up here, or whether it could make sense to relocate. If we find a better structure then maybe that results in a project. |
Transition kit to 4.0. This turns out to be a pretty small change locally, so it will probably get rolled directly into a single 2.3 -> 4.0 commit.
Follow-on bump from 4.0 to the current latest release of @solana/kit (5.4). The changes here are also surprisingly small, given there has been a major bump. This might just get rolled into a single update from 2.3 -> 5.4, subject to downstream testing.
pxrl
changed the title
pxrl
requested review from
bmzig,
chrismaree,
mrice32 and
nicholaspai
as code owners
|
@md0x @grasphoper @Reinis-FRP I've got us all the way up to Kit 5.4 in this PR. I agree that we ultimately want to move this to the SDK; I don't have the overview of the logistics of that at the moment; I think we'll plan that as a separate project. I have tested this downstream in the sdk, relayer and frontend repos; the changes required were surprisingly few. OK to get this in as-is? |
4 participants
Kit has moved from 2.2 to 5.4 in the past few months, so we're playing catch-up. I worked progressively through the major releases and found there weren't many changes required, so the jump from 2.3 -> 5.4 has been squashed into a single upgrade.
Change mostly facilitated by Claude.