-
Notifications
You must be signed in to change notification settings - Fork 9
adding ymax-alpha-1
proposals
#260
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
base: main
Are you sure you want to change the base?
Conversation
|
@mujahidkay even though this is still a draft, and can you review and provide feedback. Thanks. |
4df8e4b
to
6580a0e
Compare
}; | ||
|
||
// This test is skipped because it requires USDC which is not available until 101 passes. | ||
test.skip('ymax0 is in agoricNames.instance', async t => { |
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.
Don't skip the test.
Change the test to show that ymax0 is not in vstorage.
Also test that chain-info and access-token worked, i.e. that they put something in vstorage. As in multichain-testing
Checking for consistency of the poc brand between vbankAsset and brand is particularly helpful
Since #84 I don't think the Makefile is necessary. scripts/add-proposal.ts 100 ymax-alpha1 If the files aren't in the cache it'l prompt you to fetch them so you might have to also do, scripts/fetch-all-bundles.ts
rm -rf proposals/100*
scripts/add-proposal.ts 100 ymax-alpha1 |
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 don't see any critical problems, so I was going to approve, but I realize: this includes 101, which hasn't passed yet.
So we have to either wait or split 101 out to its own PR.
const bodyContent = parsedValue.body.slice(1); // Remove '#' prefix | ||
const brandEntries = JSON.parse(bodyContent); | ||
|
||
const poc26Entry = brandEntries.find(([name]) => name === 'PoC26'); |
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 that since we know they're entries of a keyed collection, this .find(...)
is equivalent to fromEntries(brandEntries).PoC26
The steps to parse a vstorage value... repeating them makes it hard to tell whether each repetition is doing the same thing as the others.
Consider using these utilities from ymax.test.js in the a3p-integration test:
const getCellValues = ({ value }) => {
return JSON.parse(value).values;
};
const getCapDataStructure = cell => {
const { body, slots } = JSON.parse(cell);
const structure = JSON.parse(body.replace(/^#/, ''));
return { structure, slots };
};
Then you can use something like this to get the brand
or vbankAsset
collection:
const instance = Object.fromEntries(
getCapDataStructure(getCellValues(instanceRaw).at(-1)).structure,
);
brandKeys.forEach((key, index) => { | ||
console.log(`${(index + 1).toString().padStart(2, ' ')}. ${key}`); | ||
}); | ||
console.log(`\nTotal brands: ${brandKeys.length}`); |
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 console.log
rather than t.log
?
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.
Fixed.
|
||
t.false( | ||
instanceNames.includes('ymax0'), | ||
'ymax0 should not be in vstorage yet', |
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 not" is a little strong - it should be there; maybe "no ymax0 instance in vstorage yet" or some such.
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.
Fixed.
t.log('vBank board ID:', vBankBoardId); | ||
|
||
t.is(brandBoardId, vBankBoardId, 'PoC26 brand board IDs should match'); | ||
} catch (error) { |
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 catch errors?
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.
Removed.
@@ -0,0 +1,242 @@ | |||
// @ts-check |
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 don't see anything wrong with this test, but it would be easier to be confident that it's right if it were more concise.
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 a bit concise now after using getCapDataStructure
:)
@@ -0,0 +1,47 @@ | |||
# This Makefile is here to help you find out what went on the |
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.
did you end up using add-proposal.ts
? if so, then you might as well get rid of the Makefile
.
And I'm not inclined to check tx.json
, so I have a mild preference for removing it from this PR. (not critical)
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.
Using Makefile
for now as I had already done what add-proposal.ts
is supposed to do. Will be handy next time.
echo "[$PROPOSAL] Checking if ymax0 instance is registered in agoricNames" | ||
AGORIC_NAMES=$(agd q vstorage data published.agoricNames.instance -o json) | ||
echo "$AGORIC_NAMES" | ||
|
||
if echo "$AGORIC_NAMES" | grep -q "ymax0"; then | ||
echo "[$PROPOSAL] ymax0 found in agoricNames" | ||
else | ||
echo "[$PROPOSAL] ERROR: ymax0 not found in agoricNames" | ||
exit 1 | ||
fi |
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 test; i.e. something that belongs in test.sh
.
It also looks redundant w.r.t. 101...deploy.test.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.
Yep, redundant and removed now.
@@ -0,0 +1,6 @@ | |||
# CoreEvalProposal to deploy USDC resolve to fix ymax-alpha1 |
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.
we can't land this until 101 passes
We can either
- wait
- move the 101 stuff to a separate PR and land the 100 stuff
important
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.
Yes. I don't have a preference. Let me know if you do.
}; | ||
|
||
// Retry up to 6 minutes (block time ~6s) | ||
const maxTries = 60; |
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 do we have to touch old proposals? Isn't the way their tests run fixed?
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.
For some reason, this was failing, and I added a wait, and it passed CI. But looks like it's a depot issue. Partly discussed in slack thread.
wip: updates chore: try a bunch for vstorage chore: try a bunch for vstorage chore: test ymax0 instance after usdc resolves chore: adding more tests with agd fix: lint chore: update Makefil to get tx.json
6d323b8
to
d09489c
Compare
Closes #258
adopt a passed proposal
When a proposal passes on agoric-3 Mainnet, it should be included in the history that this synthetic image tracks.
fromTag
usinglatest
(such as a3p-integration) to use a fixed version (otherwise they will fail when this PR changes latest and they pick it up)use-${proposalName}
(where proposalName is the part of the agoric-3-proposals proposal directory name after the colon, cf. a3p-integration/proposals)image building process
This should not affect other repos but be prepared that it might.
@agoric/synthetic-chain library
These need to be published to be consumed in other apps. It's not yet on 1.0 so breaking changes are allowed, but please don't make them gratuitously.
tooling improvements
As long as it passes CI it should be fine.