-
Notifications
You must be signed in to change notification settings - Fork 0
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
feat: auto install bundle on instagoric #49
base: main
Are you sure you want to change the base?
Conversation
9ecfb3d
to
d84df96
Compare
Can we have a quick working clip on PR? |
</body> | ||
</html> | ||
`); | ||
}); |
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 better to serve static HTML from a separate file.
publicapp.get('/install-bundle', (req, res) => {
res.sendFile(path.join(__dirname, 'index.html'));
});
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.
currently all static html is served from strings in this app since separate files poses some problems. we should keep it that way until we create a solution for all endpoints
body: formData | ||
}) | ||
.then(data => { | ||
alert('File uploaded successfully!'); |
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 think we should check the response status before declaring the API request as a success.
.then(response => {
if (!response.ok) {
throw new Error('Failed to upload file. Server responded with status ' + response.status);
}
return response.json();
})
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 won't be a then if status is other than 200. So response.ok
will always be true inside then.
@frazarshad Right?
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.
correct
}); | ||
|
||
publicapp.post('/install-bundle', upload.single('file'), async (req, res) => { | ||
// Getting file from request and storing as temp file |
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.
get
and post
have the same endpoint name. Do you think we should have distinct names?
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 follows the principle of rest-api where the same type of data is handled by the same endpoint, but different operations performed on it have different methods
|
||
<h1>Upload a File</h1> | ||
|
||
<form id="uploadForm" action="/install-bundle" method="POST" enctype="multipart/form-data"> |
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 need to define action
and method
here if we are overriding the default browser behavior?
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.
updated
const result = await $`\ | ||
${agBinary} tx swingset install-bundle --compress "@uploads/${bundle}" \ | ||
--from ${FAUCET_KEYNAME} --keyring-backend=test --keyring-dir=${agoricHome} --gas=auto \ | ||
--chain-id=${chainId} -b block --yes |
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 -b block
could show false positive error messages to the user
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.
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.
updated
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.
@frazarshad Regarding UI perspective, where are we exposing this option? Should it be somewhere on *.agoric.net landing page?
@Muneeb147 added a link on the main page |
|
||
const proposalPermitFile = files | ||
.filter(file => file.originalname.endsWith('permit.json')) | ||
.map(file => file.filename)[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.
If there are multiple .js and permit.json files, then it'll break as it is picking only [0]th index.
Also can we add validation if correct pair of permit.json and .js file is provided?
@toliaqat Thoughts?
Demo video:
Screen.Recording.2024-09-11.at.8.20.06.PM.mov
Instructions to run:
in agoric-sdk
bases/shared/instagoric-server/server.js
file with this codecode
in instagoric repo: