-
Notifications
You must be signed in to change notification settings - Fork 9
save proposal from archive node #84
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
Conversation
🦋 Changeset detectedLatest commit: f6e9817 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
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.
looks pretty good so far
one suggestion on fetching bundles...
"@agoric/cosmic-proto": "0.5.0-u18.5", | ||
"@agoric/cosmic-proto": "0.4.1-dev-c0e75ad.0", |
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.
why bump it backward?
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.
it's actually newer. the SDK release process bumped it, but u18 is older than dev
which is every push to master.
fixing that is something Yarn 4 and Changesets will help with
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 surprised the lerna dev publish doesn't bump versions the same way.
|
||
export const CHAINID = NonNullish(process.env.CHAINID); | ||
export const HOME = NonNullish(process.env.HOME); |
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.
so this reduces the scope of HOME
? good.
(it still bugs me that we have a file called constants.js
that's full of variables.)
console.log( | ||
'Proposal saved to', | ||
submissionDir, | ||
'. Now find these bundles and save them there too:', |
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.
bundle-tool2 might have some clues to automate this.
- provided you have the transaction hash of the bundle upload, the
txMessages
andgetBundle
calls in exploreTx should work; that's pretty well tested - to get find the transaction, queryBundleInstalls might work; it's not really used "in anger" though
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.
thanks these were critical clues
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.
so the idea is that the user fetches the bundles in a separate step?
perhaps this message should refer to scripts/fetch-all-bundles.ts
?
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.
eek, a bunch of code was lost in rebase. I'll scrounge it up
9045994
to
ef04181
Compare
This comment was marked as resolved.
This comment was marked as resolved.
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.
looks like forward progress
'@agoric/synthetic-chain': patch | ||
--- | ||
|
||
remove bankSend agd wrapper |
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.
should there be a changeset for the save feature?
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.
it's part of the repo but not what gets a versioned release
@@ -14,12 +19,10 @@ corepack enable | |||
cd "$(dirname "$(realpath -- "$0")")" | |||
|
|||
# TODO consider yarn workspaces to install all in one command | |||
if [ -n "$PROPOSAL_PATH" ]; then |
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.
my breaking change spidey-sense is tingling.
I presume this is internal, for now.
import { ProposalInfo } from './proposals.js'; | ||
import { type ProposalInfo } from './proposals.js'; |
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.
note: I wonder why yarn lint
didn't catch this earlier.
export async function saveProposalContents(proposal: ProposalInfo) { | ||
assert(isPassed(proposal), 'unpassed propoosals are not on the chain'); | ||
|
||
const tm = await makeTendermint34Client(DEFAULT_ARCHIVE_NODE, { fetch }); |
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.
note: in a follow-on, I'm inclined to try factoring out this ambient authority using some tooling...
`${proposal.proposalIdentifier}:${proposal.proposalName}`, | ||
'submission', | ||
); | ||
await fsp.mkdir(submissionDir, { recursive: true }); |
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.
likewise passing submissionDir
as a string rather than an object representing filesystem access
console.log( | ||
'Proposal saved to', | ||
submissionDir, | ||
'. Now find these bundles and save them there too:', |
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.
so the idea is that the user fetches the bundles in a separate step?
perhaps this message should refer to scripts/fetch-all-bundles.ts
?
const CACHE_DIR = path.join( | ||
process.env.HOME || process.env.USERPROFILE || '', | ||
'.agoric', | ||
'cache', | ||
); |
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.
Moving this CACHE_DIR
calculation up to scripts/fetch-all-bundles.ts
would make the data flow more clear (and avoid ambient authority).
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 tried it but then it made the cache dir configurable in a way that I didn't want. I also realized it was conflating authority with configuration. What would actually be ocap would be removing fs
and path
from the bundles
module, and creating an ad-hoc powers
API for it to use, but I don't think overall that's warranted
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.
a couple questions
scripts/fetch-all-bundles.ts
Outdated
import { fetchMsgInstallBundleTxs } from '../src/cli/chain.js'; | ||
import { refreshBundlesCache } from '../src/lib/bundles.js'; |
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.
How do these imports work?
I don't see a ../src
directory.
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.
ah, refactored without testing. will fix
import fs from 'node:fs'; | ||
import { execSync } from 'node:child_process'; | ||
|
||
import { saveProposalContents } from '../packages/synthetic-chain/src/cli/chain.ts'; |
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.
this looks like a cross-package import or something; I'm under the impression that we avoid these, though I don't completely understand why.
This doesn't rely on any undocumented magic, does it?
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.
it's bad if the importing package is published, because the file paths won't necessarily be there in other environments. but these imports are from a repo script which is never published
Co-Authored-By: Dan Connolly <[email protected]>
closes: #67
The issue was to "validate" but I figured the more valuable cases are "do it for me" and "fix it for me". This adds
scripts/add-proposal.ts
which does those.To validate, just save and
git diff
. If there's a problem, check in the correct version and figure out why SCM didn't have that.Running the tool revealed that #246 didn't have the bundles from Mainnet, which I missed in review. Not something I could have done without pulling them down, so I think that speaks to the value of the automation in this tool.